-
Notifications
You must be signed in to change notification settings - Fork 1k
Add new chunks to hypertable publication #9030
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9030 +/- ##
==========================================
+ Coverage 82.42% 82.54% +0.11%
==========================================
Files 243 243
Lines 47914 47930 +16
Branches 12229 12233 +4
==========================================
+ Hits 39492 39562 +70
- Misses 3555 3557 +2
+ Partials 4867 4811 -56 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
4dc1096 to
ace7c87
Compare
|
@pnthao, @akuzm: please review this pull request.
|
ace7c87 to
4d7d733
Compare
|
CC: @gayyappan |
efbcd81 to
0cfe55f
Compare
| INSERT INTO test_hypertable VALUES ('2024-01-03 00:00:00+00', 3, 3.0, 'data3'); | ||
|
|
||
| -- Verify (3 chunks in pub1) | ||
| SELECT schemaname, tablename, attnames, rowfilter FROM pg_publication_tables WHERE pubname = 'test_pub1' ORDER BY schemaname, tablename; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also need a test after calling compress_chunk. we should not see a new entry in the publication table.
( this also means that direct modifications to the compressed chunk will not be visible to the publication's consumer).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| chunk_insert_into_metadata_after_lock(chunk); | ||
| chunk_create_table_constraints(ht, chunk); | ||
|
|
||
| /* Add chunk to publications if hypertable is in any publications */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to make sure this doesn't happen if the chunk is a foreign table (OSM). I believe that doesn't go through this path. (I need to check the other paths that create a chunk, but wanted to raise that). The best place to add a test for OSM specific chunks is chunk_utils_internal.sql
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OSM chunks have two issues:
1. For a pre-existing publication on a hypertable, when tiering is enabled, it creates a child foreign table. This child table may be skipped when adding to the publication.
2. For a hypertable that is already tiered, adding it to a publication fails immediately because foreign tables cannot be added to a publication. I think this needs to be solved in upstream Postgres, or perhaps we could override the publishing behavior on our side(?). I want to try hacking PG code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a test for case 1.
Case 2 is an upstream issue, where they don't exclude child foreign tables.
I will try fixing upstream. Other option would be overriding CREATE/ALTER publication from our extension and changing it's behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more thing, CREATE PUBLICATION .. FOR TABLE supports ONLY option, that will skip all child tables. This can be used on our internal context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we use the ONLY workaround on the lake side.
a6e4280 to
b693f95
Compare
b693f95 to
1ce636b
Compare
|
@arajkumar @gayyappan does it remove the chunk from the publication if it is dropped either by DROP TABLE statements, drop_chunks API or retention policy? |
Yes, it should be auto-dropped. @arajkumar would be good to add a test in chunk_publication.sql using drop_chunks API. You might also consider combining chunk_publication.sql and the compression test into 1 sql test file. ( Just a suggestion). |
1ce636b to
04ee992
Compare
|
@fabriziomello @gayyappan Added tests covering drop_chunk, DROP TABLE _chunk, TRUNCATE hypertable, merge_chunk, and split_chunk. |
04ee992 to
ff319a7
Compare
|
The failure is from the following step. |
|
@fabriziomello @gayyappan PTAL. |
ff319a7 to
b7644b5
Compare
| -- Verify final state (8 chunks) | ||
| SELECT schemaname, tablename, attnames, rowfilter FROM pg_publication_tables WHERE pubname = 'test_pub' ORDER BY schemaname, tablename; | ||
|
|
||
| -- Verify chunk removal via DROP TABLE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gayyappan Here is a test that covers drop chunk in 3 scenarios.
- drop_chunk SQL api
- DROP TABLE chunk_table
- TRUNCATE TABLE
gayyappan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent test case coverage. Thank you @arajkumar
|
@fabriziomello @akuzm Could you please review this PR? Thank you. |
b7644b5 to
848d0b2
Compare
|
Please add a GUC to control this behaviour |
ac6f987 to
7fd83ce
Compare
@svenklemm Thanks for reviewing this PR. Added a GUC |
| NULL, | ||
| NULL); | ||
|
|
||
| DefineCustomBoolVariable(MAKE_EXTOPTION("enable_chunk_auto_publication"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be off by default since we dont support logical replication for hypertables and this needs additional infrastructure to work correctly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@svenklemm Ty! now it is false by default.
7fd83ce to
0401c11
Compare
Problem: Adding a hypertable to a publication automatically includes all existing chunks, but newly created chunks are not automatically added to the publication. This commit fixes the problem by adding newly created chunks to all publications that the hypertable belongs to, while respecting the hypertable’s row and column filtering rules. This ensures that logical replication works correctly for partitioned data without requiring manual chunk management. Added a new GUC timescaledb.enable_chunk_auto_publication to control behaviour of this change. By default timescaledb.enable_chunk_auto_publication is disabled, it could be modified without restarting the database. Signed-off-by: Arunprasad Rajkumar <ar.arunprasad@gmail.com>
0401c11 to
0f42a72
Compare
Problem: Adding a hypertable to a publication automatically includes all existing chunks, but newly created chunks are not automatically added to the publication.
This commit fixes the problem by adding newly created chunks to all publications that the hypertable belongs to, while respecting the hypertable’s row and column filtering rules.
This ensures that logical replication works correctly for partitioned data without requiring manual chunk management.
Added a new GUC
timescaledb.enable_chunk_auto_publicationto control behaviour of this change.By default
timescaledb.enable_chunk_auto_publicationis disabled, it could be modified without restarting the database.