($INBOX_DIR/description missing)help / color / mirror / Atom feed
[PATCH v5 1/5] Fix comments for WITH OIDs, removed at 578b22971 18+ messages / 5 participants [nested] [flat]
* [PATCH v5 1/5] Fix comments for WITH OIDs, removed at 578b22971 @ 2020-04-29 18:13 Justin Pryzby <[email protected]> 0 siblings, 0 replies; 18+ messages in thread From: Justin Pryzby @ 2020-04-29 18:13 UTC (permalink / raw) Previously mentioned here: https://www.postgresql.org/message-id/[email protected] --- src/backend/access/common/tupdesc.c | 4 +--- src/backend/access/transam/varsup.c | 3 +-- src/backend/commands/tablecmds.c | 5 ++--- 3 files changed, 4 insertions(+), 8 deletions(-) diff --git a/src/backend/access/common/tupdesc.c b/src/backend/access/common/tupdesc.c index 1e743d7d86..ce84e22cbd 100644 --- a/src/backend/access/common/tupdesc.c +++ b/src/backend/access/common/tupdesc.c @@ -786,9 +786,7 @@ TupleDescInitEntryCollation(TupleDesc desc, * * Given a relation schema (list of ColumnDef nodes), build a TupleDesc. * - * Note: the default assumption is no OIDs; caller may modify the returned - * TupleDesc if it wants OIDs. Also, tdtypeid will need to be filled in - * later on. + * tdtypeid will need to be filled in later on. */ TupleDesc BuildDescForRelation(List *schema) diff --git a/src/backend/access/transam/varsup.c b/src/backend/access/transam/varsup.c index e14b53bf9e..8328b8050a 100644 --- a/src/backend/access/transam/varsup.c +++ b/src/backend/access/transam/varsup.c @@ -527,8 +527,7 @@ GetNewObjectId(void) * The first time through this routine after normal postmaster start, the * counter will be forced up to FirstNormalObjectId. This mechanism * leaves the OIDs between FirstBootstrapObjectId and FirstNormalObjectId - * available for automatic assignment during initdb, while ensuring they - * will never conflict with user-assigned OIDs. + * available for automatic assignment during initdb. */ if (ShmemVariableCache->nextOid < ((Oid) FirstNormalObjectId)) { diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 2ab02e01a0..35945bce73 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -4829,7 +4829,7 @@ ATRewriteTables(AlterTableStmt *parsetree, List **wqueue, LOCKMODE lockmode, continue; /* - * If we change column data types or add/remove OIDs, the operation + * If we change column data types, the operation * has to be propagated to tables that use this table's rowtype as a * column type. tab->newvals will also be non-NULL in the case where * we're adding a column with a default. We choose to forbid that @@ -4853,8 +4853,7 @@ ATRewriteTables(AlterTableStmt *parsetree, List **wqueue, LOCKMODE lockmode, /* * We only need to rewrite the table if at least one column needs to - * be recomputed, we are adding/removing the OID column, or we are - * changing its persistence. + * be recomputed, or we are changing its persistence. * * There are two reasons for requiring a rewrite when changing * persistence: on one hand, we need to ensure that the buffers -- 2.17.0 --8TaQrIeukR7mmbKf Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v5-0002-Remove-mention-of-double-timestamps.patch" ^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Add an option to skip loading missing publication to avoid logical replication failure @ 2025-03-04 06:52 vignesh C <[email protected]> 0 siblings, 1 reply; 18+ messages in thread From: vignesh C @ 2025-03-04 06:52 UTC (permalink / raw) To: Amit Kapila <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]> On Mon, 3 Mar 2025 at 16:41, Amit Kapila <[email protected]> wrote: > > On Mon, Mar 3, 2025 at 2:30 PM vignesh C <[email protected]> wrote: > > > > On Tue, 25 Feb 2025 at 15:32, vignesh C <[email protected]> wrote: > > > > > > The attached script has the script that was used for testing. Here the > > > NUM_RECORDS count should be changed accordingly for each of the tests > > > and while running the test with the patch change uncomment the drop > > > publication command. > > > > I have done further analysis on the test and changed the test to > > compare it better with HEAD. The execution time is in milliseconds. > > Brach/records | 100 | 1000 | 10000 | 100000 | 1000000 > > Head | 10.43 | 15.86 | 64.44 | 550.56 | 8991.04 > > Patch | 11.35 | 17.26 | 73.50 | 640.21 | 10104.72 > > % diff | -8.82 | -8.85 | -14.08 | -16.28 | -12.38 > > > > There is a performance degradation in the range of 8.8 to 16.2 percent. > > > > - /* Validate the entry */ > - if (!entry->replicate_valid) > + /* > + * If the publication is invalid, check for updates. > + * This optimization ensures that the next block, which queries the system > + * tables and builds the relation entry, runs only if a new publication was > + * created. > + */ > + if (!publications_valid && data->publications) > + { > + bool skipped_pub = false; > + List *publications; > + > + publications = LoadPublications(data->publication_names, &skipped_pub); > > The publications_valid flag indicates whether the publications cache > is valid or not; the flag is set to false for any invalidation in the > pg_publication catalog. I wonder that instead of using the same flag > what if we use a separate publications_skipped flag? If that works, > you don't even need to change the current location where we > LoadPublications. There is almost negligible dip with the above suggested way, the test results for the same is given below(execution time is in milli seconds): Brach/records | 100 | 1000 | 10000 | 100000 | 1000000 Head | 10.25 | 15.85 | 65.53 | 569.15 | 9194.19 Patch | 10.25 | 15.84 | 65.91 | 571.75 | 9208.66 % diff | 0.00 | 0.06 | -0.58 | -0.46 | -0.16 There is a performance dip in the range of 0 to 0.58 percent. The attached patch has the changes for the same. The test script used is also attached. Regards, Vignesh Attachments: [application/x-patch] v5-0001-Fix-logical-replication-breakage-after.patch (4.4K, ../../CALDaNm2Xkm1M-ik2RLJZ9rMhW2zW2GRLL6ePyZJbXcAjOVwzXg@mail.gmail.com/2-v5-0001-Fix-logical-replication-breakage-after.patch) download | inline diff: From 8f2bca5afc03f3e34b2a0955415a702cbe6aef79 Mon Sep 17 00:00:00 2001 From: Vignesh <[email protected]> Date: Mon, 3 Mar 2025 11:54:27 +0530 Subject: [PATCH v5] Fix logical replication breakage after ALTER SUBSCRIPTION ... SET PUBLICATION Altering a subscription with `ALTER SUBSCRIPTION ... SET PUBLICATION` could cause logical replication to break under certain conditions. When the apply worker restarts after executing SET PUBLICATION, it continues using the existing replication slot and origin. If the replication origin was not updated before the restart, the WAL start location could point to a position prior to the existence of the specified publication, leading to persistent start of apply worker and reporting errors. This patch skips loading the publication if the publication does not exist and loads the publication and updates the relation entry when the publication gets created. Discussion: https://www.postgresql.org/message-id/flat/CALDaNm0-n8FGAorM%2BbTxkzn%2BAOUyx5%3DL_XmnvOP6T24%2B-NcBKg%40mail.gmail.com Discussion: https://www.postgresql.org/message-id/CAA4eK1+T-ETXeRM4DHWzGxBpKafLCp__5bPA_QZfFQp7-0wj4Q@mail.gmail.com --- src/backend/replication/pgoutput/pgoutput.c | 41 +++++++++++++++++---- 1 file changed, 33 insertions(+), 8 deletions(-) diff --git a/src/backend/replication/pgoutput/pgoutput.c b/src/backend/replication/pgoutput/pgoutput.c index 7d464f656aa..46793efe2b5 100644 --- a/src/backend/replication/pgoutput/pgoutput.c +++ b/src/backend/replication/pgoutput/pgoutput.c @@ -81,8 +81,9 @@ static void pgoutput_stream_prepare_txn(LogicalDecodingContext *ctx, ReorderBufferTXN *txn, XLogRecPtr prepare_lsn); static bool publications_valid; +static bool publications_updated; -static List *LoadPublications(List *pubnames); +static List *LoadPublications(List *pubnames, bool *skipped); static void publication_invalidation_cb(Datum arg, int cacheid, uint32 hashvalue); static void send_repl_origin(LogicalDecodingContext *ctx, @@ -1762,9 +1763,13 @@ pgoutput_shutdown(LogicalDecodingContext *ctx) /* * Load publications from the list of publication names. + * + * Here, we just skip the publications that don't exist yet. 'skipped' + * will be true if we find any publication from the given list that doesn't + * exist. */ static List * -LoadPublications(List *pubnames) +LoadPublications(List *pubnames, bool *skipped) { List *result = NIL; ListCell *lc; @@ -1772,9 +1777,15 @@ LoadPublications(List *pubnames) foreach(lc, pubnames) { char *pubname = (char *) lfirst(lc); - Publication *pub = GetPublicationByName(pubname, false); + Publication *pub = GetPublicationByName(pubname, true); - result = lappend(result, pub); + if (pub) + result = lappend(result, pub); + else + { + elog(DEBUG1, "skipped loading publication: %s", pubname); + *skipped = true; + } } return result; @@ -1789,6 +1800,7 @@ static void publication_invalidation_cb(Datum arg, int cacheid, uint32 hashvalue) { publications_valid = false; + publications_updated = true; /* * Also invalidate per-relation cache so that next time the filtering info @@ -2053,8 +2065,12 @@ get_rel_sync_entry(PGOutputData *data, Relation relation) entry->attrmap = NULL; } - /* Validate the entry */ - if (!entry->replicate_valid) + /* + * Validate the entry only if the entry is not valid or in case the + * publications have been updated. + */ + if (!entry->replicate_valid || + (!publications_valid && publications_updated)) { Oid schemaId = get_rel_namespace(relid); List *pubids = GetRelationPublications(relid); @@ -2071,16 +2087,25 @@ get_rel_sync_entry(PGOutputData *data, Relation relation) bool am_partition = get_rel_relispartition(relid); char relkind = get_rel_relkind(relid); List *rel_publications = NIL; + bool skipped_pub = false; /* Reload publications if needed before use. */ if (!publications_valid) { + publications_updated = false; + MemoryContextReset(data->pubctx); oldctx = MemoryContextSwitchTo(data->pubctx); - data->publications = LoadPublications(data->publication_names); + data->publications = LoadPublications(data->publication_names, &skipped_pub); MemoryContextSwitchTo(oldctx); - publications_valid = true; + + /* + * We don't consider the publications to be valid till we have + * information of all the publications. + */ + if (!skipped_pub) + publications_valid = true; } /* -- 2.43.0 [text/x-sh] test_pg_recvlogical.sh (1.3K, ../../CALDaNm2Xkm1M-ik2RLJZ9rMhW2zW2GRLL6ePyZJbXcAjOVwzXg@mail.gmail.com/3-test_pg_recvlogical.sh) download | inline: #!/bin/bash ##### SLOT_NAME=test PLUGIN_NAME=pgoutput NUM_RECORDS=100000 LOOP=10 ##### for i in `seq 1 $LOOP` do # Cleanup previous result pg_ctl stop -D data rm -rf data logfile # Initialize an instance initdb -D data -U postgres -c wal_level=logical # Start the instance pg_ctl -D data -l logfile start # Create a table psql -U postgres -c "CREATE TABLE foo (id int);" psql -U postgres -c "CREATE publication pub1 for table foo;" psql -U postgres -c "CREATE publication pub2 for table foo;" #psql -U postgres -c "drop publication pub1;" # Create a replication slot psql -U postgres -c "SELECT * FROM pg_create_logical_replication_slot('$SLOT_NAME', '$PLUGIN_NAME')" # Insert tuples (this transaction will be decoded) psql -U postgres -c "INSERT INTO foo VALUES (generate_series(1, $NUM_RECORDS))" # Confirm current WAL location WAL_POS=$(psql -qtAX -U postgres -c "SELECT * FROM pg_current_wal_lsn()") t1=$(($(date +%s%N)/1000)) echo $t1 > run_${i}.dat # Run pg_recvlogical till the current WAL location time pg_recvlogical -d postgres -U postgres --start -S $SLOT_NAME -E $WAL_POS -f - -o publication_names='pub1,pub2' -o proto_version=4 ) &>> run_${i}.dat t2=$(($(date +%s%N)/1000)) echo $t2 >> run_${i}.dat t3=$((t2-t1)) echo "execution time=$t3" >> run_${i}.dat done ^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Add an option to skip loading missing publication to avoid logical replication failure @ 2025-03-04 13:24 vignesh C <[email protected]> parent: vignesh C <[email protected]> 0 siblings, 1 reply; 18+ messages in thread From: vignesh C @ 2025-03-04 13:24 UTC (permalink / raw) To: Amit Kapila <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]> On Tue, 4 Mar 2025 at 12:22, vignesh C <[email protected]> wrote: > > On Mon, 3 Mar 2025 at 16:41, Amit Kapila <[email protected]> wrote: > > > > On Mon, Mar 3, 2025 at 2:30 PM vignesh C <[email protected]> wrote: > > > > > > On Tue, 25 Feb 2025 at 15:32, vignesh C <[email protected]> wrote: > > > > > > > > The attached script has the script that was used for testing. Here the > > > > NUM_RECORDS count should be changed accordingly for each of the tests > > > > and while running the test with the patch change uncomment the drop > > > > publication command. > > > > > > I have done further analysis on the test and changed the test to > > > compare it better with HEAD. The execution time is in milliseconds. > > > Brach/records | 100 | 1000 | 10000 | 100000 | 1000000 > > > Head | 10.43 | 15.86 | 64.44 | 550.56 | 8991.04 > > > Patch | 11.35 | 17.26 | 73.50 | 640.21 | 10104.72 > > > % diff | -8.82 | -8.85 | -14.08 | -16.28 | -12.38 > > > > > > There is a performance degradation in the range of 8.8 to 16.2 percent. > > > > > > > - /* Validate the entry */ > > - if (!entry->replicate_valid) > > + /* > > + * If the publication is invalid, check for updates. > > + * This optimization ensures that the next block, which queries the system > > + * tables and builds the relation entry, runs only if a new publication was > > + * created. > > + */ > > + if (!publications_valid && data->publications) > > + { > > + bool skipped_pub = false; > > + List *publications; > > + > > + publications = LoadPublications(data->publication_names, &skipped_pub); > > > > The publications_valid flag indicates whether the publications cache > > is valid or not; the flag is set to false for any invalidation in the > > pg_publication catalog. I wonder that instead of using the same flag > > what if we use a separate publications_skipped flag? If that works, > > you don't even need to change the current location where we > > LoadPublications. > > There is almost negligible dip with the above suggested way, the test > results for the same is given below(execution time is in milli > seconds): > Brach/records | 100 | 1000 | 10000 | 100000 | 1000000 > Head | 10.25 | 15.85 | 65.53 | 569.15 | 9194.19 > Patch | 10.25 | 15.84 | 65.91 | 571.75 | 9208.66 > % diff | 0.00 | 0.06 | -0.58 | -0.46 | -0.16 > > There is a performance dip in the range of 0 to 0.58 percent. > The attached patch has the changes for the same. The test script used > is also attached. On further thinking, I felt the use of publications_updated variable is not required we can use publications_valid itself which will be set if the publication system table is invalidated. Here is a patch for the same. Regards, Vignesh Attachments: [text/x-patch] v6-0001-Fix-logical-replication-breakage-after-ALTER-SUBS.patch (2.6K, ../../CALDaNm3Ub=T1c78kDF3y5Wcna3WrzoiEeVL8_atbSFSiVhj8FQ@mail.gmail.com/2-v6-0001-Fix-logical-replication-breakage-after-ALTER-SUBS.patch) download | inline diff: From 856403e4943954267b0e9b4fa96ccc6955838210 Mon Sep 17 00:00:00 2001 From: Vignesh <[email protected]> Date: Mon, 3 Mar 2025 11:54:27 +0530 Subject: [PATCH v6] Fix logical replication breakage after ALTER SUBSCRIPTION ... SET PUBLICATION Altering a subscription with `ALTER SUBSCRIPTION ... SET PUBLICATION` could cause logical replication to break under certain conditions. When the apply worker restarts after executing SET PUBLICATION, it continues using the existing replication slot and origin. If the replication origin was not updated before the restart, the WAL start location could point to a position prior to the existence of the specified publication, leading to persistent start of apply worker and reporting errors. This patch skips loading the publication if the publication does not exist and loads the publication and updates the relation entry when the publication gets created. Discussion: https://www.postgresql.org/message-id/flat/CALDaNm0-n8FGAorM%2BbTxkzn%2BAOUyx5%3DL_XmnvOP6T24%2B-NcBKg%40mail.gmail.com Discussion: https://www.postgresql.org/message-id/CAA4eK1+T-ETXeRM4DHWzGxBpKafLCp__5bPA_QZfFQp7-0wj4Q@mail.gmail.com --- src/backend/replication/pgoutput/pgoutput.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/src/backend/replication/pgoutput/pgoutput.c b/src/backend/replication/pgoutput/pgoutput.c index 7d464f656aa..4cfd2b02023 100644 --- a/src/backend/replication/pgoutput/pgoutput.c +++ b/src/backend/replication/pgoutput/pgoutput.c @@ -1762,6 +1762,8 @@ pgoutput_shutdown(LogicalDecodingContext *ctx) /* * Load publications from the list of publication names. + * + * Here, we just skip the publications that don't exist yet. */ static List * LoadPublications(List *pubnames) @@ -1772,9 +1774,12 @@ LoadPublications(List *pubnames) foreach(lc, pubnames) { char *pubname = (char *) lfirst(lc); - Publication *pub = GetPublicationByName(pubname, false); + Publication *pub = GetPublicationByName(pubname, true); - result = lappend(result, pub); + if (pub) + result = lappend(result, pub); + else + elog(WARNING, "skipped loading publication: %s", pubname); } return result; @@ -2053,8 +2058,11 @@ get_rel_sync_entry(PGOutputData *data, Relation relation) entry->attrmap = NULL; } - /* Validate the entry */ - if (!entry->replicate_valid) + /* + * Validate the entry if (a) the entry is not valid, or (b) there is a + * change in the publications. + */ + if (!entry->replicate_valid || !publications_valid) { Oid schemaId = get_rel_namespace(relid); List *pubids = GetRelationPublications(relid); -- 2.43.0 ^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Add an option to skip loading missing publication to avoid logical replication failure @ 2025-03-10 04:02 Amit Kapila <[email protected]> parent: vignesh C <[email protected]> 0 siblings, 2 replies; 18+ messages in thread From: Amit Kapila @ 2025-03-10 04:02 UTC (permalink / raw) To: vignesh C <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]> On Tue, Mar 4, 2025 at 6:54 PM vignesh C <[email protected]> wrote: > > On further thinking, I felt the use of publications_updated variable > is not required we can use publications_valid itself which will be set > if the publication system table is invalidated. Here is a patch for > the same. > The patch relies on the fact that whenever a publication's data is invalidated, it will also invalidate all the RelSyncEntires as per publication_invalidation_cb. But note that we are discussing removing that inefficiency in the thread [1]. So, we should try to rebuild the entry when we have skipped the required publication previously. Apart from this, please consider updating the docs, as mentioned in my response to Sawada-San's email. BTW, I am planning to commit this only on HEAD as this is a behavior change. Please let me know if you guys think otherwise. [1] - https://www.postgresql.org/message-id/OSCPR01MB14966C09AA201EFFA706576A7F5C92%40OSCPR01MB14966.jpnpr... -- With Regards, Amit Kapila. ^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Add an option to skip loading missing publication to avoid logical replication failure @ 2025-03-10 04:44 Dilip Kumar <[email protected]> parent: Amit Kapila <[email protected]> 1 sibling, 1 reply; 18+ messages in thread From: Dilip Kumar @ 2025-03-10 04:44 UTC (permalink / raw) To: Amit Kapila <[email protected]>; +Cc: vignesh C <[email protected]>; PostgreSQL Hackers <[email protected]> On Mon, Mar 10, 2025 at 9:33 AM Amit Kapila <[email protected]> wrote: > On Tue, Mar 4, 2025 at 6:54 PM vignesh C <[email protected]> wrote: > > > > On further thinking, I felt the use of publications_updated variable > > is not required we can use publications_valid itself which will be set > > if the publication system table is invalidated. Here is a patch for > > the same. > > > > The patch relies on the fact that whenever a publication's data is > invalidated, it will also invalidate all the RelSyncEntires as per > publication_invalidation_cb. But note that we are discussing removing > that inefficiency in the thread [1]. So, we should try to rebuild the > entry when we have skipped the required publication previously. > > Apart from this, please consider updating the docs, as mentioned in my > response to Sawada-San's email. > I'm not sure I fully understand it, but based on your previous email and the initial email from Vignesh, if IIUC, the issue occurs when a publication is created after a certain LSN. When ALTER SUBSCRIPTION ... SET PUBLICATION is executed, the subscriber workers restart and request the changes based on restart_lsn, which is at an earlier LSN in the WAL than the LSN at which the publication was created. This leads to an error, and we are addressing this behavior as part of the fix by skipping the changes which are between the restart_lsn of subscriber and the lsn at which publication is created and this behavior looks fine. BTW, I am planning to commit this only on HEAD as this is a behavior > change. Please let me know if you guys think otherwise. > Somehow this looks like a bug fix which should be backported no? Am I missing something? -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Add an option to skip loading missing publication to avoid logical replication failure @ 2025-03-10 05:24 Amit Kapila <[email protected]> parent: Dilip Kumar <[email protected]> 0 siblings, 1 reply; 18+ messages in thread From: Amit Kapila @ 2025-03-10 05:24 UTC (permalink / raw) To: Dilip Kumar <[email protected]>; +Cc: vignesh C <[email protected]>; PostgreSQL Hackers <[email protected]> On Mon, Mar 10, 2025 at 10:15 AM Dilip Kumar <[email protected]> wrote: > > On Mon, Mar 10, 2025 at 9:33 AM Amit Kapila <[email protected]> wrote: >> >> On Tue, Mar 4, 2025 at 6:54 PM vignesh C <[email protected]> wrote: >> > >> > On further thinking, I felt the use of publications_updated variable >> > is not required we can use publications_valid itself which will be set >> > if the publication system table is invalidated. Here is a patch for >> > the same. >> > >> >> The patch relies on the fact that whenever a publication's data is >> invalidated, it will also invalidate all the RelSyncEntires as per >> publication_invalidation_cb. But note that we are discussing removing >> that inefficiency in the thread [1]. So, we should try to rebuild the >> entry when we have skipped the required publication previously. >> >> Apart from this, please consider updating the docs, as mentioned in my >> response to Sawada-San's email. > > > I'm not sure I fully understand it, but based on your previous email and the initial email from Vignesh, if IIUC, the issue occurs when a publication is created after a certain LSN. When ALTER SUBSCRIPTION ... SET PUBLICATION is executed, the subscriber workers restart and request the changes based on restart_lsn, which is at an earlier LSN in the WAL than the LSN at which the publication was created. This leads to an error, and we are addressing this behavior as part of the fix by skipping the changes which are between the restart_lsn of subscriber and the lsn at which publication is created and this behavior looks fine. > Yes, your understanding is correct, but note that as such, the patch is simply skipping the missing publication. The skipped changes are because those were on the table that is not part of any publication w.r.t historic snapshot we have at the point of time. >> BTW, I am planning to commit this only on HEAD as this is a behavior >> change. Please let me know if you guys think otherwise. > > > Somehow this looks like a bug fix which should be backported no? Am I missing something? > We can consider this a bug-fix and backpatch it, but I am afraid that because this is a behavior change, some users may not like it. Also, I don't remember seeing public reports for this behavior; that is probably because it is hard to hit. FYI, we found this via BF failures. So, I thought it would be better to get this field tested via HEAD only at this point in time. -- With Regards, Amit Kapila. ^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Add an option to skip loading missing publication to avoid logical replication failure @ 2025-03-11 04:17 Dilip Kumar <[email protected]> parent: Amit Kapila <[email protected]> 0 siblings, 1 reply; 18+ messages in thread From: Dilip Kumar @ 2025-03-11 04:17 UTC (permalink / raw) To: Amit Kapila <[email protected]>; +Cc: vignesh C <[email protected]>; PostgreSQL Hackers <[email protected]> On Mon, Mar 10, 2025 at 10:54 AM Amit Kapila <[email protected]> wrote: > > On Mon, Mar 10, 2025 at 10:15 AM Dilip Kumar <[email protected]> wrote: > > > > On Mon, Mar 10, 2025 at 9:33 AM Amit Kapila <[email protected]> wrote: > >> > >> On Tue, Mar 4, 2025 at 6:54 PM vignesh C <[email protected]> wrote: > >> > > >> > On further thinking, I felt the use of publications_updated variable > >> > is not required we can use publications_valid itself which will be set > >> > if the publication system table is invalidated. Here is a patch for > >> > the same. > >> > > >> > >> The patch relies on the fact that whenever a publication's data is > >> invalidated, it will also invalidate all the RelSyncEntires as per > >> publication_invalidation_cb. But note that we are discussing removing > >> that inefficiency in the thread [1]. So, we should try to rebuild the > >> entry when we have skipped the required publication previously. > >> > >> Apart from this, please consider updating the docs, as mentioned in my > >> response to Sawada-San's email. > > > > > > I'm not sure I fully understand it, but based on your previous email and the initial email from Vignesh, if IIUC, the issue occurs when a publication is created after a certain LSN. When ALTER SUBSCRIPTION ... SET PUBLICATION is executed, the subscriber workers restart and request the changes based on restart_lsn, which is at an earlier LSN in the WAL than the LSN at which the publication was created. This leads to an error, and we are addressing this behavior as part of the fix by skipping the changes which are between the restart_lsn of subscriber and the lsn at which publication is created and this behavior looks fine. > > > > Yes, your understanding is correct, but note that as such, the patch > is simply skipping the missing publication. The skipped changes are > because those were on the table that is not part of any publication > w.r.t historic snapshot we have at the point of time. So, it will skip loading the missing publication up to the LSN where the publication is created and then load it from there, correct? Do we have a test case for this? I couldn't find one in the latest patch or in the email thread to demonstrate this behavior. > >> BTW, I am planning to commit this only on HEAD as this is a behavior > >> change. Please let me know if you guys think otherwise. > > > > > > Somehow this looks like a bug fix which should be backported no? Am I missing something? > > > > We can consider this a bug-fix and backpatch it, but I am afraid that > because this is a behavior change, some users may not like it. Also, I > don't remember seeing public reports for this behavior; that is > probably because it is hard to hit. FYI, we found this via BF > failures. So, I thought it would be better to get this field tested > via HEAD only at this point in time. At the moment, I don't have a strong opinion on this. Since no one has encountered or reported this issue, it might be the case that it's not affecting anyone, and we could simply backpatch without causing any dissatisfaction. However, I'm fine with whatever others decide. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Add an option to skip loading missing publication to avoid logical replication failure @ 2025-03-11 10:30 vignesh C <[email protected]> parent: Amit Kapila <[email protected]> 1 sibling, 1 reply; 18+ messages in thread From: vignesh C @ 2025-03-11 10:30 UTC (permalink / raw) To: Amit Kapila <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]> On Mon, 10 Mar 2025 at 09:33, Amit Kapila <[email protected]> wrote: > > On Tue, Mar 4, 2025 at 6:54 PM vignesh C <[email protected]> wrote: > > > > On further thinking, I felt the use of publications_updated variable > > is not required we can use publications_valid itself which will be set > > if the publication system table is invalidated. Here is a patch for > > the same. > > > > The patch relies on the fact that whenever a publication's data is > invalidated, it will also invalidate all the RelSyncEntires as per > publication_invalidation_cb. But note that we are discussing removing > that inefficiency in the thread [1]. So, we should try to rebuild the > entry when we have skipped the required publication previously. > > Apart from this, please consider updating the docs, as mentioned in my > response to Sawada-San's email. The create subscription documentation already has "We allow non-existent publications to be specified so that users can add those later. This means pg_subscription can have non-existent publications." and should be enough at [1]. Let me know if we need to add more documentation. Apart from this I have changed the log level that logs "skipped loading publication" to WARNING as we log a warning "WARNING: publications "pub2", "pub3" do not exist on the publisher" in case of CREATE SUBSCRIPTION and looked similar to this. I can change it to a different log level in case you feel this is not the right level. Also I have added a test case for dilip's comment from [2]. The attached v7 version patch has the changes for the same. [1] - https://www.postgresql.org/docs/devel/sql-createsubscription.html [2] - https://www.postgresql.org/message-id/CAFiTN-tgUR6QLSs3UHK7gx4VP7cURGNkufA_xkrQLw9eCnbGQw%40mail.gma... Regards, Vignesh Attachments: [application/octet-stream] v7-0001-Fix-logical-replication-breakage-after-ALTER-SUBS.patch (5.0K, ../../CALDaNm0T4uP0BTQHAzsmw3EtCoq=-y4Ryww9PZTrqiZeyA80XQ@mail.gmail.com/2-v7-0001-Fix-logical-replication-breakage-after-ALTER-SUBS.patch) download | inline diff: From 1b838710fbbe16d412c5eff8a38d1aeb5590dd23 Mon Sep 17 00:00:00 2001 From: Vignesh <[email protected]> Date: Mon, 3 Mar 2025 11:54:27 +0530 Subject: [PATCH v7] Fix logical replication breakage after ALTER SUBSCRIPTION ... SET PUBLICATION Altering a subscription with `ALTER SUBSCRIPTION ... SET PUBLICATION` could cause logical replication to break under certain conditions. When the apply worker restarts after executing SET PUBLICATION, it continues using the existing replication slot and origin. If the replication origin was not updated before the restart, the WAL start location could point to a position prior to the existence of the specified publication, leading to persistent start of apply worker and reporting errors. This patch skips loading the publication if the publication does not exist and loads the publication and updates the relation entry when the publication gets created. Discussion: https://www.postgresql.org/message-id/flat/CALDaNm0-n8FGAorM%2BbTxkzn%2BAOUyx5%3DL_XmnvOP6T24%2B-NcBKg%40mail.gmail.com Discussion: https://www.postgresql.org/message-id/CAA4eK1+T-ETXeRM4DHWzGxBpKafLCp__5bPA_QZfFQp7-0wj4Q@mail.gmail.com --- src/backend/replication/pgoutput/pgoutput.c | 26 ++++++++++++++++-- src/test/subscription/t/024_add_drop_pub.pl | 29 ++++++++++++++++++++- 2 files changed, 52 insertions(+), 3 deletions(-) diff --git a/src/backend/replication/pgoutput/pgoutput.c b/src/backend/replication/pgoutput/pgoutput.c index 9063af6e1df..991aa6f7282 100644 --- a/src/backend/replication/pgoutput/pgoutput.c +++ b/src/backend/replication/pgoutput/pgoutput.c @@ -1762,6 +1762,8 @@ pgoutput_shutdown(LogicalDecodingContext *ctx) /* * Load publications from the list of publication names. + * + * Here, we just skip the publications that don't exist yet. */ static List * LoadPublications(List *pubnames) @@ -1772,9 +1774,29 @@ LoadPublications(List *pubnames) foreach(lc, pubnames) { char *pubname = (char *) lfirst(lc); - Publication *pub = GetPublicationByName(pubname, false); + Publication *pub = GetPublicationByName(pubname, true); - result = lappend(result, pub); + if (pub) + result = lappend(result, pub); + else + { + /* + * When executing 'ALTER SUBSCRIPTION ... SET PUBLICATION', the + * apply worker continues using the existing replication slot and + * origin after restarting. If the replication origin is not + * updated before the restart, the WAL start location may point to + * a position before the specified publication exists, causing + * persistent apply worker restarts and errors. + * + * This ensures that the publication is skipped if it does not + * exist and is loaded when the corresponding WAL record is + * encountered. + */ + ereport(WARNING, + errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), + errmsg("skipped loading publication: %s", pubname), + errhint("If the publication already exists, ignore it as it will be loaded upon reaching the corresponding WAL record; otherwise, create it.")); + } } return result; diff --git a/src/test/subscription/t/024_add_drop_pub.pl b/src/test/subscription/t/024_add_drop_pub.pl index 4428a3413db..dfdc9835e0c 100644 --- a/src/test/subscription/t/024_add_drop_pub.pl +++ b/src/test/subscription/t/024_add_drop_pub.pl @@ -1,7 +1,9 @@ # Copyright (c) 2021-2025, PostgreSQL Global Development Group -# This test checks behaviour of ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION +# This test checks behaviour of ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION and +# ensures that dropping a publication associated with a subscription does not +# disrupt existing logical replication. use strict; use warnings FATAL => 'all'; use PostgreSQL::Test::Cluster; @@ -80,6 +82,31 @@ $result = $node_subscriber->safe_psql('postgres', "SELECT count(*), min(a), max(a) FROM tab_1"); is($result, qq(20|1|10), 'check initial data is copied to subscriber'); +# Ensure that dropping a publication associated with a subscription does not +# disrupt existing logical replication. Instead, it should log a warning +# while allowing replication to continue. Additionally, verify that replication +# of the dropped publication resumes once the publication is recreated. +$node_publisher->safe_psql('postgres', "DROP PUBLICATION tap_pub_1;"); + +my $offset = -s $node_publisher->logfile; + +$node_publisher->safe_psql('postgres', "INSERT INTO tab_1 values(50)"); + +# Verify that a warning is logged. +$node_publisher->wait_for_log( + qr/WARNING: ( [A-Z0-9]+:)? skipped loading publication: tap_pub_1/, $offset); + +$node_publisher->safe_psql('postgres', + "CREATE PUBLICATION tap_pub_1 FOR TABLE tab_1"); + +$node_publisher->safe_psql('postgres', "INSERT INTO tab_1 values(11)"); + +# Verify that the insert operation gets replicated to subscriber after +# re-createion. +$result = $node_subscriber->safe_psql('postgres', + "SELECT count(*), min(a), max(a) FROM tab_1"); +is($result, qq(21|1|11), 'check that the incremental data is replicated after the dropped publication is re-created'); + # shutdown $node_subscriber->stop('fast'); $node_publisher->stop('fast'); -- 2.43.0 ^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Add an option to skip loading missing publication to avoid logical replication failure @ 2025-03-12 10:34 Amit Kapila <[email protected]> parent: Dilip Kumar <[email protected]> 0 siblings, 1 reply; 18+ messages in thread From: Amit Kapila @ 2025-03-12 10:34 UTC (permalink / raw) To: Dilip Kumar <[email protected]>; +Cc: vignesh C <[email protected]>; PostgreSQL Hackers <[email protected]> On Tue, Mar 11, 2025 at 9:48 AM Dilip Kumar <[email protected]> wrote: > > On Mon, Mar 10, 2025 at 10:54 AM Amit Kapila <[email protected]> wrote: > > > > > >> BTW, I am planning to commit this only on HEAD as this is a behavior > > >> change. Please let me know if you guys think otherwise. > > > > > > > > > Somehow this looks like a bug fix which should be backported no? Am I missing something? > > > > > > > We can consider this a bug-fix and backpatch it, but I am afraid that > > because this is a behavior change, some users may not like it. Also, I > > don't remember seeing public reports for this behavior; that is > > probably because it is hard to hit. FYI, we found this via BF > > failures. So, I thought it would be better to get this field tested > > via HEAD only at this point in time. > > At the moment, I don't have a strong opinion on this. Since no one has > encountered or reported this issue, it might be the case that it's not > affecting anyone, and we could simply backpatch without causing any > dissatisfaction. However, I'm fine with whatever others decide. > Sawada-San, others, do you have an opinion on whether to backpatch this change? -- With Regards, Amit Kapila. ^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Add an option to skip loading missing publication to avoid logical replication failure @ 2025-03-12 10:45 Dilip Kumar <[email protected]> parent: vignesh C <[email protected]> 0 siblings, 1 reply; 18+ messages in thread From: Dilip Kumar @ 2025-03-12 10:45 UTC (permalink / raw) To: vignesh C <[email protected]>; +Cc: Amit Kapila <[email protected]>; PostgreSQL Hackers <[email protected]> On Tue, Mar 11, 2025 at 4:01 PM vignesh C <[email protected]> wrote: > > On Mon, 10 Mar 2025 at 09:33, Amit Kapila <[email protected]> wrote: > > > > On Tue, Mar 4, 2025 at 6:54 PM vignesh C <[email protected]> wrote: > > > > > > On further thinking, I felt the use of publications_updated variable > > > is not required we can use publications_valid itself which will be set > > > if the publication system table is invalidated. Here is a patch for > > > the same. > > > > > > > The patch relies on the fact that whenever a publication's data is > > invalidated, it will also invalidate all the RelSyncEntires as per > > publication_invalidation_cb. But note that we are discussing removing > > that inefficiency in the thread [1]. So, we should try to rebuild the > > entry when we have skipped the required publication previously. > > > > Apart from this, please consider updating the docs, as mentioned in my > > response to Sawada-San's email. > > The create subscription documentation already has "We allow > non-existent publications to be specified so that users can add those > later. This means pg_subscription can have non-existent publications." > and should be enough at [1]. Let me know if we need to add more > documentation. > > Apart from this I have changed the log level that logs "skipped > loading publication" to WARNING as we log a warning "WARNING: > publications "pub2", "pub3" do not exist on the publisher" in case of > CREATE SUBSCRIPTION and looked similar to this. I can change it to a > different log level in case you feel this is not the right level. > > Also I have added a test case for dilip's comment from [2]. > The attached v7 version patch has the changes for the same. > Thanks, Vignesh, for adding the test. I believe you've tested the effect of DROP PUBLICATION. However, I think we should also test the behavior of ALTER SUBSCRIPTION...SET PUBLICATION before creating the PUBLICATION, and then create the PUBLICATION at a later stage. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Add an option to skip loading missing publication to avoid logical replication failure @ 2025-03-12 21:51 Masahiko Sawada <[email protected]> parent: Amit Kapila <[email protected]> 0 siblings, 0 replies; 18+ messages in thread From: Masahiko Sawada @ 2025-03-12 21:51 UTC (permalink / raw) To: Amit Kapila <[email protected]>; +Cc: Dilip Kumar <[email protected]>; vignesh C <[email protected]>; PostgreSQL Hackers <[email protected]> On Wed, Mar 12, 2025 at 3:34 AM Amit Kapila <[email protected]> wrote: > > On Tue, Mar 11, 2025 at 9:48 AM Dilip Kumar <[email protected]> wrote: > > > > On Mon, Mar 10, 2025 at 10:54 AM Amit Kapila <[email protected]> wrote: > > > > > > > > >> BTW, I am planning to commit this only on HEAD as this is a behavior > > > >> change. Please let me know if you guys think otherwise. > > > > > > > > > > > > Somehow this looks like a bug fix which should be backported no? Am I missing something? > > > > > > > > > > We can consider this a bug-fix and backpatch it, but I am afraid that > > > because this is a behavior change, some users may not like it. Also, I > > > don't remember seeing public reports for this behavior; that is > > > probably because it is hard to hit. FYI, we found this via BF > > > failures. So, I thought it would be better to get this field tested > > > via HEAD only at this point in time. > > > > At the moment, I don't have a strong opinion on this. Since no one has > > encountered or reported this issue, it might be the case that it's not > > affecting anyone, and we could simply backpatch without causing any > > dissatisfaction. However, I'm fine with whatever others decide. > > > > Sawada-San, others, do you have an opinion on whether to backpatch this change? I'm also afraid of backpatching it so I guess it would be better to push it to only HEAD. I think if users have encountered and we see reported the issue we can consider backpatching again. If regression tests on backbranches continue to fail intermittently, probably we can consider adding waits as the patch Osumi-san proposed[1]? Regards, [1] https://www.postgresql.org/message-id/TYCPR01MB83737A68CD5D554EA82BD7B9EDD39%40TYCPR01MB8373.jpnprd0... -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Add an option to skip loading missing publication to avoid logical replication failure @ 2025-03-13 02:08 vignesh C <[email protected]> parent: Dilip Kumar <[email protected]> 0 siblings, 1 reply; 18+ messages in thread From: vignesh C @ 2025-03-13 02:08 UTC (permalink / raw) To: Dilip Kumar <[email protected]>; +Cc: Amit Kapila <[email protected]>; PostgreSQL Hackers <[email protected]> On Wed, 12 Mar 2025 at 16:15, Dilip Kumar <[email protected]> wrote: > > Thanks, Vignesh, for adding the test. I believe you've tested the > effect of DROP PUBLICATION. However, I think we should also test the > behavior of ALTER SUBSCRIPTION...SET PUBLICATION before creating the > PUBLICATION, and then create the PUBLICATION at a later stage. I felt having only one test case for this is enough, I have removed the DROP PUBLICATION test and added the SET PUBLICATION test. The attached v8 version patch has the changes for the same. Regards, Vignesh Attachments: [text/x-patch] v8-0001-Fix-logical-replication-breakage-after-ALTER-SUBS.patch (5.5K, ../../CALDaNm3HYP5WJLV9cKWEXuGgWYSLFOs-=u0qf5OD4Zyb87=6gw@mail.gmail.com/2-v8-0001-Fix-logical-replication-breakage-after-ALTER-SUBS.patch) download | inline diff: From c2f806ce866e3b0dfcebcd639683627a83e4e35a Mon Sep 17 00:00:00 2001 From: Vignesh <[email protected]> Date: Mon, 3 Mar 2025 11:54:27 +0530 Subject: [PATCH v8] Fix logical replication breakage after ALTER SUBSCRIPTION ... SET PUBLICATION Altering a subscription with `ALTER SUBSCRIPTION ... SET PUBLICATION` could cause logical replication to break under certain conditions. When the apply worker restarts after executing SET PUBLICATION, it continues using the existing replication slot and origin. If the replication origin was not updated before the restart, the WAL start location could point to a position prior to the existence of the specified publication, leading to persistent start of apply worker and reporting errors. This patch skips loading the publication if the publication does not exist and loads the publication and updates the relation entry when the publication gets created. Discussion: https://www.postgresql.org/message-id/flat/CALDaNm0-n8FGAorM%2BbTxkzn%2BAOUyx5%3DL_XmnvOP6T24%2B-NcBKg%40mail.gmail.com Discussion: https://www.postgresql.org/message-id/CAA4eK1+T-ETXeRM4DHWzGxBpKafLCp__5bPA_QZfFQp7-0wj4Q@mail.gmail.com --- src/backend/replication/pgoutput/pgoutput.c | 26 +++++++++++- src/test/subscription/t/024_add_drop_pub.pl | 44 ++++++++++++++++++++- 2 files changed, 67 insertions(+), 3 deletions(-) diff --git a/src/backend/replication/pgoutput/pgoutput.c b/src/backend/replication/pgoutput/pgoutput.c index 9063af6e1df..991aa6f7282 100644 --- a/src/backend/replication/pgoutput/pgoutput.c +++ b/src/backend/replication/pgoutput/pgoutput.c @@ -1762,6 +1762,8 @@ pgoutput_shutdown(LogicalDecodingContext *ctx) /* * Load publications from the list of publication names. + * + * Here, we just skip the publications that don't exist yet. */ static List * LoadPublications(List *pubnames) @@ -1772,9 +1774,29 @@ LoadPublications(List *pubnames) foreach(lc, pubnames) { char *pubname = (char *) lfirst(lc); - Publication *pub = GetPublicationByName(pubname, false); + Publication *pub = GetPublicationByName(pubname, true); - result = lappend(result, pub); + if (pub) + result = lappend(result, pub); + else + { + /* + * When executing 'ALTER SUBSCRIPTION ... SET PUBLICATION', the + * apply worker continues using the existing replication slot and + * origin after restarting. If the replication origin is not + * updated before the restart, the WAL start location may point to + * a position before the specified publication exists, causing + * persistent apply worker restarts and errors. + * + * This ensures that the publication is skipped if it does not + * exist and is loaded when the corresponding WAL record is + * encountered. + */ + ereport(WARNING, + errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), + errmsg("skipped loading publication: %s", pubname), + errhint("If the publication already exists, ignore it as it will be loaded upon reaching the corresponding WAL record; otherwise, create it.")); + } } return result; diff --git a/src/test/subscription/t/024_add_drop_pub.pl b/src/test/subscription/t/024_add_drop_pub.pl index 4428a3413db..4eb17f45ec3 100644 --- a/src/test/subscription/t/024_add_drop_pub.pl +++ b/src/test/subscription/t/024_add_drop_pub.pl @@ -1,7 +1,9 @@ # Copyright (c) 2021-2025, PostgreSQL Global Development Group -# This test checks behaviour of ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION +# This test checks behaviour of ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION and +# ensures that dropping a publication associated with a subscription does not +# disrupt existing logical replication. use strict; use warnings FATAL => 'all'; use PostgreSQL::Test::Cluster; @@ -80,6 +82,46 @@ $result = $node_subscriber->safe_psql('postgres', "SELECT count(*), min(a), max(a) FROM tab_1"); is($result, qq(20|1|10), 'check initial data is copied to subscriber'); +# Ensure that setting a missing publication to the subscription does not +# disrupt existing logical replication. Instead, it should log a warning +# while allowing replication to continue. Additionally, verify that replication +# resumes after the missing publication is created for the publication table. + +# Create table on publisher and subscriber +$node_publisher->safe_psql('postgres', "CREATE TABLE tab_3 (a int)"); +$node_subscriber->safe_psql('postgres', "CREATE TABLE tab_3 (a int)"); + +# Set the subscription with a missing publication +$node_subscriber->safe_psql('postgres', + "ALTER SUBSCRIPTION tap_sub SET PUBLICATION tap_pub_3"); + +my $offset = -s $node_publisher->logfile; + +$node_publisher->safe_psql('postgres',"INSERT INTO tab_3 values(1)"); + +# Verify that a warning is logged. +$node_publisher->wait_for_log( + qr/WARNING: ( [A-Z0-9]+:)? skipped loading publication: tap_pub_3/, $offset); + +$node_publisher->safe_psql('postgres', + "CREATE PUBLICATION tap_pub_3 FOR TABLE tab_3"); + +$node_subscriber->safe_psql('postgres', + "ALTER SUBSCRIPTION tap_sub REFRESH PUBLICATION"); + +$node_subscriber->wait_for_subscription_sync($node_publisher, 'tap_sub'); + +$node_publisher->safe_psql('postgres', "INSERT INTO tab_3 values(2)"); + +$node_publisher->wait_for_catchup('tap_sub'); + +# Verify that the insert operation gets replicated to subscriber after +# publication is created. +$result = $node_subscriber->safe_psql('postgres', + "SELECT * FROM tab_3"); +is($result, qq(1 +2), 'check that the incremental data is replicated after the publication is created'); + # shutdown $node_subscriber->stop('fast'); $node_publisher->stop('fast'); -- 2.43.0 ^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Add an option to skip loading missing publication to avoid logical replication failure @ 2025-03-13 03:48 Dilip Kumar <[email protected]> parent: vignesh C <[email protected]> 0 siblings, 1 reply; 18+ messages in thread From: Dilip Kumar @ 2025-03-13 03:48 UTC (permalink / raw) To: vignesh C <[email protected]>; +Cc: Amit Kapila <[email protected]>; PostgreSQL Hackers <[email protected]> On Thu, Mar 13, 2025 at 7:38 AM vignesh C <[email protected]> wrote: > > On Wed, 12 Mar 2025 at 16:15, Dilip Kumar <[email protected]> wrote: > > > > Thanks, Vignesh, for adding the test. I believe you've tested the > > effect of DROP PUBLICATION. However, I think we should also test the > > behavior of ALTER SUBSCRIPTION...SET PUBLICATION before creating the > > PUBLICATION, and then create the PUBLICATION at a later stage. > > I felt having only one test case for this is enough, I have removed > the DROP PUBLICATION test and added the SET PUBLICATION test. The > attached v8 version patch has the changes for the same. Thanks looks good to me. While looking at the patch, I have a few comments/questions + if (pub) + result = lappend(result, pub); + else + { + /* + * When executing 'ALTER SUBSCRIPTION ... SET PUBLICATION', the + * apply worker continues using the existing replication slot and + * origin after restarting. If the replication origin is not + * updated before the restart, the WAL start location may point to + * a position before the specified publication exists, causing + * persistent apply worker restarts and errors. + * + * This ensures that the publication is skipped if it does not + * exist and is loaded when the corresponding WAL record is + * encountered. + */ + ereport(WARNING, + errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), + errmsg("skipped loading publication: %s", pubname), + errhint("If the publication already exists, ignore it as it will be loaded upon reaching the corresponding WAL record; otherwise, create it.")); + } This comment focuses on a specific use case regarding the problem with 'ALTER SUBSCRIPTION ... SET PUBLICATION,' but in reality, we are addressing a more general case where the user is trying to SET PUBLICATION or even CREATE SUBSCRIPTION, and some publications are missing. Wouldn't it be better to rephrase the comment? 2. + errhint("If the publication already exists, ignore it as it will be loaded upon reaching the corresponding WAL record; otherwise, create it.")); Is this hint correct? This is a question rather than a comment: When we reach a particular WAL where the publication was created, will the publication automatically load, or does the user need to REFRESH the publications? -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Add an option to skip loading missing publication to avoid logical replication failure @ 2025-03-13 05:19 vignesh C <[email protected]> parent: Dilip Kumar <[email protected]> 0 siblings, 1 reply; 18+ messages in thread From: vignesh C @ 2025-03-13 05:19 UTC (permalink / raw) To: Dilip Kumar <[email protected]>; +Cc: Amit Kapila <[email protected]>; PostgreSQL Hackers <[email protected]> On Thu, 13 Mar 2025 at 09:18, Dilip Kumar <[email protected]> wrote: > > Thanks looks good to me. > > While looking at the patch, I have a few comments/questions > > + if (pub) > + result = lappend(result, pub); > + else > + { > + /* > + * When executing 'ALTER SUBSCRIPTION ... SET PUBLICATION', the > + * apply worker continues using the existing replication slot and > + * origin after restarting. If the replication origin is not > + * updated before the restart, the WAL start location may point to > + * a position before the specified publication exists, causing > + * persistent apply worker restarts and errors. > + * > + * This ensures that the publication is skipped if it does not > + * exist and is loaded when the corresponding WAL record is > + * encountered. > + */ > + ereport(WARNING, > + errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), > + errmsg("skipped loading publication: %s", pubname), > + errhint("If the publication already exists, ignore it as it will be > loaded upon reaching the corresponding WAL record; otherwise, create > it.")); > + } > > This comment focuses on a specific use case regarding the problem with > 'ALTER SUBSCRIPTION ... SET PUBLICATION,' but in reality, we are > addressing a more general case where the user is trying to SET > PUBLICATION or even CREATE SUBSCRIPTION, and some publications are > missing. Wouldn't it be better to rephrase the comment? How about a comment something like below: /* * In 'ALTER SUBSCRIPTION ... ADD/SET PUBLICATION' and * 'CREATE SUBSCRIPTION', if the replication origin is not updated * before the worker exits, the WAL start location might point to a * position before the publication's WAL record. This can lead to * persistent apply worker restarts and errors. * * Additionally, dropping a subscription's publication should not * disrupt logical replication. * * This ensures that a missing publication is skipped and loaded * when its corresponding WAL record is encountered. */ ereport(WARNING, errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), errmsg("skipped loading publication: %s", pubname), errhint("If the publication is missing, create and refresh it. Otherwise, wait for the slot to reach the WAL record, then refresh")); > 2. + errhint("If the publication already exists, ignore it as it will > be loaded upon reaching the corresponding WAL record; otherwise, > create it.")); > > Is this hint correct? This is a question rather than a comment: When > we reach a particular WAL where the publication was created, will the > publication automatically load, or does the user need to REFRESH the > publications? Users need to refresh the publication in case the relation is not already added to pg_subscription_rel and apply incremental changes. How about an error hint like: "If the publication is missing, create and refresh it. Otherwise, wait for the slot to reach the WAL record for the created publication, then refresh" Regards, Vignesh ^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Add an option to skip loading missing publication to avoid logical replication failure @ 2025-03-13 06:13 Dilip Kumar <[email protected]> parent: vignesh C <[email protected]> 0 siblings, 1 reply; 18+ messages in thread From: Dilip Kumar @ 2025-03-13 06:13 UTC (permalink / raw) To: vignesh C <[email protected]>; +Cc: Amit Kapila <[email protected]>; PostgreSQL Hackers <[email protected]> On Thu, Mar 13, 2025 at 10:49 AM vignesh C <[email protected]> wrote: > > On Thu, 13 Mar 2025 at 09:18, Dilip Kumar <[email protected]> wrote: > > > > Thanks looks good to me. > > > > While looking at the patch, I have a few comments/questions > > > > + if (pub) > > + result = lappend(result, pub); > > + else > > + { > > + /* > > + * When executing 'ALTER SUBSCRIPTION ... SET PUBLICATION', the > > + * apply worker continues using the existing replication slot and > > + * origin after restarting. If the replication origin is not > > + * updated before the restart, the WAL start location may point to > > + * a position before the specified publication exists, causing > > + * persistent apply worker restarts and errors. > > + * > > + * This ensures that the publication is skipped if it does not > > + * exist and is loaded when the corresponding WAL record is > > + * encountered. > > + */ > > + ereport(WARNING, > > + errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), > > + errmsg("skipped loading publication: %s", pubname), > > + errhint("If the publication already exists, ignore it as it will be > > loaded upon reaching the corresponding WAL record; otherwise, create > > it.")); > > + } > > > > This comment focuses on a specific use case regarding the problem with > > 'ALTER SUBSCRIPTION ... SET PUBLICATION,' but in reality, we are > > addressing a more general case where the user is trying to SET > > PUBLICATION or even CREATE SUBSCRIPTION, and some publications are > > missing. Wouldn't it be better to rephrase the comment? > > How about a comment something like below: > /* > * In 'ALTER SUBSCRIPTION ... ADD/SET PUBLICATION' and > * 'CREATE SUBSCRIPTION', if the replication origin is not updated > * before the worker exits, the WAL start location might point to a > * position before the publication's WAL record. This can lead to > * persistent apply worker restarts and errors. > * > * Additionally, dropping a subscription's publication should not > * disrupt logical replication. > * > * This ensures that a missing publication is skipped and loaded > * when its corresponding WAL record is encountered. > */ Looks fine, shall we add the missing publication point as well something like below /* * In operations like 'ALTER SUBSCRIPTION ... ADD/SET PUBLICATION' and * 'CREATE SUBSCRIPTION', if the specified publication does not exist or * if the replication origin is not updated before the worker exits, * the WAL start location may point to a position prior to the publication's * WAL record. This can cause persistent restarts and errors * in the apply worker. * > * Additionally, dropping a subscription's publication should not > * disrupt logical replication. * > * This ensures that a missing publication is skipped and loaded > * when its corresponding WAL record is encountered. */ > ereport(WARNING, > errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), > errmsg("skipped loading publication: %s", pubname), > errhint("If the publication is missing, create and refresh it. > Otherwise, wait for the slot to reach the WAL record, then refresh")); > > > 2. + errhint("If the publication already exists, ignore it as it will > > be loaded upon reaching the corresponding WAL record; otherwise, > > create it.")); > > > > Is this hint correct? This is a question rather than a comment: When > > we reach a particular WAL where the publication was created, will the > > publication automatically load, or does the user need to REFRESH the > > publications? > > Users need to refresh the publication in case the relation is not > already added to pg_subscription_rel and apply incremental changes. > How about an error hint like: > "If the publication is missing, create and refresh it. Otherwise, wait > for the slot to reach the WAL record for the created publication, then > refresh" +1 -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Add an option to skip loading missing publication to avoid logical replication failure @ 2025-03-13 09:50 Amit Kapila <[email protected]> parent: Dilip Kumar <[email protected]> 0 siblings, 1 reply; 18+ messages in thread From: Amit Kapila @ 2025-03-13 09:50 UTC (permalink / raw) To: Dilip Kumar <[email protected]>; +Cc: vignesh C <[email protected]>; PostgreSQL Hackers <[email protected]> On Thu, Mar 13, 2025 at 11:43 AM Dilip Kumar <[email protected]> wrote: > > Looks fine, shall we add the missing publication point as well > something like below > > /* > * In operations like 'ALTER SUBSCRIPTION ... ADD/SET PUBLICATION' and > * 'CREATE SUBSCRIPTION', if the specified publication does not exist or > * if the replication origin is not updated before the worker exits, > * the WAL start location may point to a position prior to the publication's > * WAL record. This can cause persistent restarts and errors > * in the apply worker. > * I think that is too much related to pub-sub model, and ideally, pgoutput should not care about it. I have written a comment considering somebody using pgoutput decoding module via APIs. > > * Additionally, dropping a subscription's publication should not > > * disrupt logical replication. > * > > * This ensures that a missing publication is skipped and loaded > > * when its corresponding WAL record is encountered. > */ > > > > > ereport(WARNING, > > errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), > > errmsg("skipped loading publication: %s", pubname), > > errhint("If the publication is missing, create and refresh it. > > Otherwise, wait for the slot to reach the WAL record, then refresh")); > > > > > 2. + errhint("If the publication already exists, ignore it as it will > > > be loaded upon reaching the corresponding WAL record; otherwise, > > > create it.")); > > > > > > Is this hint correct? This is a question rather than a comment: When > > > we reach a particular WAL where the publication was created, will the > > > publication automatically load, or does the user need to REFRESH the > > > publications? > > > > Users need to refresh the publication in case the relation is not > > already added to pg_subscription_rel and apply incremental changes. > > How about an error hint like: > > "If the publication is missing, create and refresh it. Otherwise, wait > > for the slot to reach the WAL record for the created publication, then > > refresh" > I have tried to split this information into errdetail and errhint in the attached. See and let me know what you think of the same. -- With Regards, Amit Kapila. Attachments: [application/octet-stream] v9-0001-Fix-ALTER-SUBSCRIPTION-.-SET-PUBLICATION-.-comman.patch (5.6K, ../../CAA4eK1L-4c2+v7NZB4jWDwtN-VT+UKAm9Vp2dzO2Tyg0HTJvbQ@mail.gmail.com/2-v9-0001-Fix-ALTER-SUBSCRIPTION-.-SET-PUBLICATION-.-comman.patch) download | inline diff: From de767940fe6f33bb8e12975428b827cfd183dcd5 Mon Sep 17 00:00:00 2001 From: Vignesh <[email protected]> Date: Mon, 3 Mar 2025 11:54:27 +0530 Subject: [PATCH v9] Fix ALTER SUBSCRIPTION ... SET PUBLICATION ... command. The problem is that ALTER SUBSCRIPTION ... SET PUBLICATION ... will lead to restarting of apply worker and after the restart, the apply worker will use the existing slot and replication origin corresponding to the subscription. Now, it is possible that before the restart, the origin has not been updated, and the WAL start location points to a location before where PUBLICATION pointed to by SET PUBLICATION doesn't exist, and that can lead to an error like: "ERROR: publication "pub1" does not exist". Once this error occurs, apply worker will never be able to proceed and will always return the same error. We decided to skip loading the publication if the publication does not exist. The publication is loaded later and updates the relation entry when the publication gets created. We decided not to backpatch this as this is a behaviour change, and we don't see field reports. This problem has been found by intermittent buildfarm failures. Author: vignesh C <[email protected]> Reviewed-by: Dilip Kumar <[email protected]> Reviewed-by: Amit Kapila <[email protected]> Reviewed-by: Masahiko Sawada <[email protected]> Discussion: https://postgr.es/m/flat/CALDaNm0-n8FGAorM%2BbTxkzn%2BAOUyx5%3DL_XmnvOP6T24%2B-NcBKg%40mail.gmail.com Discussion: https://postgr.es/m/CAA4eK1+T-ETXeRM4DHWzGxBpKafLCp__5bPA_QZfFQp7-0wj4Q@mail.gmail.com --- src/backend/replication/pgoutput/pgoutput.c | 16 +++++++- src/test/subscription/t/024_add_drop_pub.pl | 44 ++++++++++++++++++++- 2 files changed, 57 insertions(+), 3 deletions(-) diff --git a/src/backend/replication/pgoutput/pgoutput.c b/src/backend/replication/pgoutput/pgoutput.c index ed806c54300..8357bf8b4c0 100644 --- a/src/backend/replication/pgoutput/pgoutput.c +++ b/src/backend/replication/pgoutput/pgoutput.c @@ -1764,6 +1764,11 @@ pgoutput_shutdown(LogicalDecodingContext *ctx) /* * Load publications from the list of publication names. + * + * Here, we skip the publications that don't exist yet. This will allow us + * to silently continue the replication in the absence of a missing publication. + * This is required because we allow the users to create publications after they + * have specified the required publications at the time of replication start. */ static List * LoadPublications(List *pubnames) @@ -1774,9 +1779,16 @@ LoadPublications(List *pubnames) foreach(lc, pubnames) { char *pubname = (char *) lfirst(lc); - Publication *pub = GetPublicationByName(pubname, false); + Publication *pub = GetPublicationByName(pubname, true); - result = lappend(result, pub); + if (pub) + result = lappend(result, pub); + else + ereport(WARNING, + errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), + errmsg("skipped loading publication: %s", pubname), + errdetail("The publication does not exist at this point in the WAL."), + errhint("Create the publication if it does not exist.")); } return result; diff --git a/src/test/subscription/t/024_add_drop_pub.pl b/src/test/subscription/t/024_add_drop_pub.pl index 4428a3413db..b594941c7cb 100644 --- a/src/test/subscription/t/024_add_drop_pub.pl +++ b/src/test/subscription/t/024_add_drop_pub.pl @@ -1,7 +1,9 @@ # Copyright (c) 2021-2025, PostgreSQL Global Development Group -# This test checks behaviour of ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION +# This test checks behaviour of ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION and +# ensures that creating a publication associated with a subscription at a later +# point of time does not break logical replication. use strict; use warnings FATAL => 'all'; use PostgreSQL::Test::Cluster; @@ -80,6 +82,46 @@ $result = $node_subscriber->safe_psql('postgres', "SELECT count(*), min(a), max(a) FROM tab_1"); is($result, qq(20|1|10), 'check initial data is copied to subscriber'); +# Ensure that setting a missing publication to the subscription does not +# disrupt existing logical replication. Instead, it should log a warning +# while allowing replication to continue. Additionally, verify that replication +# resumes after the missing publication is created for the publication table. + +# Create table on publisher and subscriber +$node_publisher->safe_psql('postgres', "CREATE TABLE tab_3 (a int)"); +$node_subscriber->safe_psql('postgres', "CREATE TABLE tab_3 (a int)"); + +# Set the subscription with a missing publication +$node_subscriber->safe_psql('postgres', + "ALTER SUBSCRIPTION tap_sub SET PUBLICATION tap_pub_3"); + +my $offset = -s $node_publisher->logfile; + +$node_publisher->safe_psql('postgres',"INSERT INTO tab_3 values(1)"); + +# Verify that a warning is logged. +$node_publisher->wait_for_log( + qr/WARNING: ( [A-Z0-9]+:)? skipped loading publication: tap_pub_3/, $offset); + +$node_publisher->safe_psql('postgres', + "CREATE PUBLICATION tap_pub_3 FOR TABLE tab_3"); + +$node_subscriber->safe_psql('postgres', + "ALTER SUBSCRIPTION tap_sub REFRESH PUBLICATION"); + +$node_subscriber->wait_for_subscription_sync($node_publisher, 'tap_sub'); + +$node_publisher->safe_psql('postgres', "INSERT INTO tab_3 values(2)"); + +$node_publisher->wait_for_catchup('tap_sub'); + +# Verify that the insert operation gets replicated to subscriber after +# publication is created. +$result = $node_subscriber->safe_psql('postgres', + "SELECT * FROM tab_3"); +is($result, qq(1 +2), 'check that the incremental data is replicated after the publication is created'); + # shutdown $node_subscriber->stop('fast'); $node_publisher->stop('fast'); -- 2.28.0.windows.1 ^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Add an option to skip loading missing publication to avoid logical replication failure @ 2025-03-13 13:56 Dilip Kumar <[email protected]> parent: Amit Kapila <[email protected]> 0 siblings, 1 reply; 18+ messages in thread From: Dilip Kumar @ 2025-03-13 13:56 UTC (permalink / raw) To: Amit Kapila <[email protected]>; +Cc: vignesh C <[email protected]>; PostgreSQL Hackers <[email protected]> On Thu, Mar 13, 2025 at 3:20 PM Amit Kapila <[email protected]> wrote: > > On Thu, Mar 13, 2025 at 11:43 AM Dilip Kumar <[email protected]> wrote: > > > > Looks fine, shall we add the missing publication point as well > > something like below > > > > /* > > * In operations like 'ALTER SUBSCRIPTION ... ADD/SET PUBLICATION' and > > * 'CREATE SUBSCRIPTION', if the specified publication does not exist or > > * if the replication origin is not updated before the worker exits, > > * the WAL start location may point to a position prior to the publication's > > * WAL record. This can cause persistent restarts and errors > > * in the apply worker. > > * > > I think that is too much related to pub-sub model, and ideally, > pgoutput should not care about it. I have written a comment > considering somebody using pgoutput decoding module via APIs. I agree, here we just need to talk about skipping the missing publication, not different scenarios where that can happen. This comments look much better. > > > * Additionally, dropping a subscription's publication should not > > > * disrupt logical replication. > > * > > > * This ensures that a missing publication is skipped and loaded > > > * when its corresponding WAL record is encountered. > > */ > > > > > > > > > ereport(WARNING, > > > errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), > > > errmsg("skipped loading publication: %s", pubname), > > > errhint("If the publication is missing, create and refresh it. > > > Otherwise, wait for the slot to reach the WAL record, then refresh")); > > > > > > > 2. + errhint("If the publication already exists, ignore it as it will > > > > be loaded upon reaching the corresponding WAL record; otherwise, > > > > create it.")); > > > > > > > > Is this hint correct? This is a question rather than a comment: When > > > > we reach a particular WAL where the publication was created, will the > > > > publication automatically load, or does the user need to REFRESH the > > > > publications? > > > > > > Users need to refresh the publication in case the relation is not > > > already added to pg_subscription_rel and apply incremental changes. > > > How about an error hint like: > > > "If the publication is missing, create and refresh it. Otherwise, wait > > > for the slot to reach the WAL record for the created publication, then > > > refresh" > > > > I have tried to split this information into errdetail and errhint in > the attached. See and let me know what you think of the same. Yes, the errhint also makes sense to me. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Add an option to skip loading missing publication to avoid logical replication failure @ 2025-03-14 08:58 Amit Kapila <[email protected]> parent: Dilip Kumar <[email protected]> 0 siblings, 0 replies; 18+ messages in thread From: Amit Kapila @ 2025-03-14 08:58 UTC (permalink / raw) To: Dilip Kumar <[email protected]>; +Cc: vignesh C <[email protected]>; PostgreSQL Hackers <[email protected]> On Thu, Mar 13, 2025 at 7:26 PM Dilip Kumar <[email protected]> wrote: > > On Thu, Mar 13, 2025 at 3:20 PM Amit Kapila <[email protected]> wrote: > > > > > > I think that is too much related to pub-sub model, and ideally, > > pgoutput should not care about it. I have written a comment > > considering somebody using pgoutput decoding module via APIs. > Thanks, Dilip and Sawada-San, for the inputs, and Vignesh for the patch. I have pushed the change. -- With Regards, Amit Kapila. ^ permalink raw reply [nested|flat] 18+ messages in thread
end of thread, other threads:[~2025-03-14 08:58 UTC | newest] Thread overview: 18+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-04-29 18:13 [PATCH v5 1/5] Fix comments for WITH OIDs, removed at 578b22971 Justin Pryzby <[email protected]> 2025-03-04 06:52 Re: Add an option to skip loading missing publication to avoid logical replication failure vignesh C <[email protected]> 2025-03-04 13:24 ` Re: Add an option to skip loading missing publication to avoid logical replication failure vignesh C <[email protected]> 2025-03-10 04:02 ` Re: Add an option to skip loading missing publication to avoid logical replication failure Amit Kapila <[email protected]> 2025-03-10 04:44 ` Re: Add an option to skip loading missing publication to avoid logical replication failure Dilip Kumar <[email protected]> 2025-03-10 05:24 ` Re: Add an option to skip loading missing publication to avoid logical replication failure Amit Kapila <[email protected]> 2025-03-11 04:17 ` Re: Add an option to skip loading missing publication to avoid logical replication failure Dilip Kumar <[email protected]> 2025-03-12 10:34 ` Re: Add an option to skip loading missing publication to avoid logical replication failure Amit Kapila <[email protected]> 2025-03-12 21:51 ` Re: Add an option to skip loading missing publication to avoid logical replication failure Masahiko Sawada <[email protected]> 2025-03-11 10:30 ` Re: Add an option to skip loading missing publication to avoid logical replication failure vignesh C <[email protected]> 2025-03-12 10:45 ` Re: Add an option to skip loading missing publication to avoid logical replication failure Dilip Kumar <[email protected]> 2025-03-13 02:08 ` Re: Add an option to skip loading missing publication to avoid logical replication failure vignesh C <[email protected]> 2025-03-13 03:48 ` Re: Add an option to skip loading missing publication to avoid logical replication failure Dilip Kumar <[email protected]> 2025-03-13 05:19 ` Re: Add an option to skip loading missing publication to avoid logical replication failure vignesh C <[email protected]> 2025-03-13 06:13 ` Re: Add an option to skip loading missing publication to avoid logical replication failure Dilip Kumar <[email protected]> 2025-03-13 09:50 ` Re: Add an option to skip loading missing publication to avoid logical replication failure Amit Kapila <[email protected]> 2025-03-13 13:56 ` Re: Add an option to skip loading missing publication to avoid logical replication failure Dilip Kumar <[email protected]> 2025-03-14 08:58 ` Re: Add an option to skip loading missing publication to avoid logical replication failure Amit Kapila <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox