-
Notifications
You must be signed in to change notification settings - Fork 695
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
CDC implementation for Citus using Logical Replication #6623
Conversation
src/backend/distributed/utils/replication_origin_session_utils.c
Outdated
Show resolved
Hide resolved
src/backend/distributed/utils/replication_origin_session_utils.c
Outdated
Show resolved
Hide resolved
src/backend/distributed/utils/replication_origin_session_utils.c
Outdated
Show resolved
Hide resolved
src/backend/distributed/utils/replication_origin_session_utils.c
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## main #6623 +/- ##
==========================================
+ Coverage 93.14% 93.16% +0.01%
==========================================
Files 260 262 +2
Lines 56140 56327 +187
==========================================
+ Hits 52293 52478 +185
- Misses 3847 3849 +2 |
Previously trigger type was pull_request and events were opened, reopened and synchronize. In PR #6623, packaging pipeline was not triggered and PR was blocked . It may be a bug in GH Actions side. To make sure of execution of the pipeline, I changed pipeline execution type into 'branches'
70a88d8
to
a9b2a59
Compare
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.
test approach makes sense to me, for the basic test it would be good to also include update/delete
src/test/regress/expected/citus_cdc_basic_distributed_table.out
Outdated
Show resolved
Hide resolved
src/test/regress/expected/citus_cdc_basic_distributed_table.out
Outdated
Show resolved
Hide resolved
src/backend/distributed/utils/replication_origin_session_utils.c
Outdated
Show resolved
Hide resolved
src/backend/distributed/utils/replication_origin_session_utils.c
Outdated
Show resolved
Hide resolved
src/backend/distributed/utils/replication_origin_session_utils.c
Outdated
Show resolved
Hide resolved
src/backend/distributed/utils/replication_origin_session_utils.c
Outdated
Show resolved
Hide resolved
src/backend/distributed/utils/replication_origin_session_utils.c
Outdated
Show resolved
Hide resolved
src/backend/distributed/utils/replication_origin_session_utils.c
Outdated
Show resolved
Hide resolved
src/backend/distributed/utils/replication_origin_session_utils.c
Outdated
Show resolved
Hide resolved
src/backend/distributed/utils/replication_origin_session_utils.c
Outdated
Show resolved
Hide resolved
I'm getting this error a lot when something fails and presumably the decoder restarts:
this is mainly related to splits, can we avoid it somehow? |
typedef struct | ||
{ | ||
Oid shardId; | ||
Oid distributedTableId; |
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.
Even after combining with #6776 we're no longer seeing inserts after an alter_distributed_table, not sure why, but we can look at it later.
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.
Maybe SetupReplicationOriginLocalSession
need to go into ConvertTable
itself so that we cover all different conversions?
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.
alter_distribute_table changes the Oid of the distributed table, so the CDC client's mapping is no longer valid and hence any inserts/update/deletes after that, is not able to be applied by the CDC subscriber. The only current option is for the subscriber to drop the subscription and recreate the subscription again. This will be documented as a known issue.
I guess we can improve the title of the PR |
src/test/cdc/t/002_cdc_create_distributed_table_concurrently.pl
Outdated
Show resolved
Hide resolved
I have changed the Title now, please check if it is ok.. |
src/test/regress/expected/citus_cdc_basic_distributed_table_concurrently.out
Outdated
Show resolved
Hide resolved
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 looks pretty good to me. Huge leap forward in terms of setting up CDC for Citus!
I think we can do the "unbundling" of the decoder plugin as a separate PR.
"_PG_output_plugin_init", | ||
false, NULL); | ||
(LogicalOutputPluginInit) (void *) | ||
load_external_function("pgoutput", |
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.
maybe make it a compile-time constant for now (ideally one that can be overridden via -D option)
Added 3 library dependencies for CDC TAP tests: libdbi-perl libdbd-pg-perl postgresql-${PG_MAJOR}-wal2json This is needed for dependencies added for these PRs: citusdata/citus#6827 citusdata/citus#6623
DESCRIPTION: Implementing CDC changes using Logical Replication to avoid re-publishing events multiple times by setting up replication origin session, which will add "DoNotReplicateId" to every WAL entry.
The citus decoder which will be decoding WAL events for CDC clients,
ignores any WAL entry with replication origin that is not zero.
It also maps the shard names to distributed table names.