Hi Peter, Vignesh, Thanks for the review. Attached is v3, based on the same master commit: 77705d1e5a616339c3e92f586bbfa7b6bb562beb.
Vignesh wrote: > 1) Can we create the table in publisher and subscriber outside the > loop instead of doing it twice, similarly the publication can be > created outside the loop: Done. > 2) With the first changes, we can truncate the tables instead of dropping > them: Done. The subscription is dropped before truncating the tables between modes. Each mode still checks initial sync and an incremental insert. Peter wrote: > Saying publish = 'insert' seems overkill. Removed. > Those could be combined a bit... I've combined the cleanup and tidied the comments, spacing and semicolons as suggested in your attachment. The C code is unchanged from v2. Vignesh wrote: > 3) Till which branch will this patch be applied? I reproduced the text-mode issue on PG10. I haven't reproduced it on the other versions, but the code appears to have the same problem. The binary case appears to be affected from PG16, which added binary initial sync. I think we should backpatch to the affected supported branches. I have only tested the patch on master so far. I tested v3 on master with assertions enabled. The regression tests and the relevant logical replication tests passed. Best regards, mingjiegao
From 641fa7649f8aae270c022e64cff39d7c04d41adb Mon Sep 17 00:00:00 2001 From: mingjiegao <[email protected]> Date: Fri, 6 Nov 2026 17:03:12 +0800 Subject: [PATCH v3] Fix initial sync of zero-column published tables Initial table sync treats an empty publisher column list as an unspecified COPY column list. When the subscriber has additional columns, this can insert an empty string instead of a local default in text mode, or fail with a field-count error in binary mode. Allow COPY initialization to distinguish an explicitly empty column list from an unspecified one, and use this in tablesync. --- Changes since v2: - Create the zero-column test tables and publication once, outside the text/binary loop, and reset the tables with TRUNCATE after dropping each subscription. - Remove the unnecessary publish option and consolidate cleanup. - Tidy test comments, spacing, and SQL statement terminators. src/backend/commands/copyfrom.c | 49 ++++++++++++---- src/backend/replication/logical/tablesync.c | 3 +- src/include/commands/copy.h | 5 ++ src/test/subscription/t/008_diff_schema.pl | 65 +++++++++++++++++++++ 4 files changed, 111 insertions(+), 11 deletions(-) diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.c index 746b4fdf6c3..d8d09a543f0 100644 --- a/src/backend/commands/copyfrom.c +++ b/src/backend/commands/copyfrom.c @@ -1518,6 +1518,27 @@ CopyFrom(CopyFromState cstate) return processed; } +/* + * Wrapper for BeginCopyFromExtended() with attnamelist_is_explicit = false. + * + * A NIL attnamelist selects all target columns that are neither dropped nor + * generated. + */ +CopyFromState +BeginCopyFrom(ParseState *pstate, + Relation rel, + Node *whereClause, + const char *filename, + bool is_program, + copy_data_source_cb data_source_cb, + List *attnamelist, + List *options) +{ + return BeginCopyFromExtended(pstate, rel, whereClause, filename, + is_program, data_source_cb, attnamelist, + options, false); +} + /* * Setup to read tuples from a file for COPY FROM. * @@ -1526,20 +1547,25 @@ CopyFrom(CopyFromState cstate) * 'filename': Name of server-local file to read, NULL for STDIN * 'is_program': true if 'filename' is program to execute * 'data_source_cb': callback that provides the input data - * 'attnamelist': List of char *, columns to include. NIL selects all cols. + * 'attnamelist': List of char *, columns to include. * 'options': List of DefElem. See copy_opt_item in gram.y for selections. + * 'attnamelist_is_explicit': If true, a NIL attnamelist means zero input + * columns. Otherwise, NIL selects all target columns that are neither + * dropped nor generated. This is needed when a logical replication source + * has no columns. * * Returns a CopyFromState, to be passed to NextCopyFrom and related functions. */ CopyFromState -BeginCopyFrom(ParseState *pstate, - Relation rel, - Node *whereClause, - const char *filename, - bool is_program, - copy_data_source_cb data_source_cb, - List *attnamelist, - List *options) +BeginCopyFromExtended(ParseState *pstate, + Relation rel, + Node *whereClause, + const char *filename, + bool is_program, + copy_data_source_cb data_source_cb, + List *attnamelist, + List *options, + bool attnamelist_is_explicit) { CopyFromState cstate; bool pipe = (filename == NULL); @@ -1590,7 +1616,10 @@ BeginCopyFrom(ParseState *pstate, /* process common options or initialization */ /* Generate or convert list of attributes to process */ - cstate->attnumlist = CopyGetAttnums(tupDesc, cstate->rel, attnamelist); + if (attnamelist_is_explicit && attnamelist == NIL) + cstate->attnumlist = NIL; + else + cstate->attnumlist = CopyGetAttnums(tupDesc, cstate->rel, attnamelist); num_phys_attrs = tupDesc->natts; diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c index 4015e861a64..723012fb0c0 100644 --- a/src/backend/replication/logical/tablesync.c +++ b/src/backend/replication/logical/tablesync.c @@ -1211,7 +1211,8 @@ copy_table(Relation rel) NULL, false, false); attnamelist = make_copy_attnamelist(relmapentry); - cstate = BeginCopyFrom(pstate, rel, NULL, NULL, false, copy_read_data, attnamelist, options); + cstate = BeginCopyFromExtended(pstate, rel, NULL, NULL, false, + copy_read_data, attnamelist, options, true); /* Do the copy */ (void) CopyFrom(cstate); diff --git a/src/include/commands/copy.h b/src/include/commands/copy.h index abecfe51098..74203f3547f 100644 --- a/src/include/commands/copy.h +++ b/src/include/commands/copy.h @@ -114,6 +114,11 @@ extern void ProcessCopyOptions(ParseState *pstate, CopyFormatOptions *opts_out, extern CopyFromState BeginCopyFrom(ParseState *pstate, Relation rel, Node *whereClause, const char *filename, bool is_program, copy_data_source_cb data_source_cb, List *attnamelist, List *options); +extern CopyFromState BeginCopyFromExtended(ParseState *pstate, Relation rel, + Node *whereClause, const char *filename, + bool is_program, copy_data_source_cb data_source_cb, + List *attnamelist, List *options, + bool attnamelist_is_explicit); extern void EndCopyFrom(CopyFromState cstate); extern bool NextCopyFrom(CopyFromState cstate, ExprContext *econtext, Datum *values, bool *nulls); diff --git a/src/test/subscription/t/008_diff_schema.pl b/src/test/subscription/t/008_diff_schema.pl index 96e3d6f5888..5482b69c451 100644 --- a/src/test/subscription/t/008_diff_schema.pl +++ b/src/test/subscription/t/008_diff_schema.pl @@ -31,6 +31,71 @@ $node_subscriber->safe_psql('postgres', # Setup logical replication my $publisher_connstr = $node_publisher->connstr . ' dbname=postgres'; + +# A zero-column publisher still sends rows. Subscriber-only columns must +# get their defaults during initial COPY as well as incremental replication. +$node_publisher->safe_psql( + 'postgres', q{ + CREATE TABLE tab_nocols (); + CREATE PUBLICATION pub_nocols FOR TABLE tab_nocols; +}); +$node_subscriber->safe_psql('postgres', + "CREATE TABLE tab_nocols (local_value text DEFAULT 'subscriber-default');" +); + +foreach my $binary ('false', 'true') +{ + $node_publisher->safe_psql( + 'postgres', q{ + INSERT INTO tab_nocols DEFAULT VALUES; + INSERT INTO tab_nocols DEFAULT VALUES; + }); + $node_subscriber->safe_psql('postgres', + "CREATE SUBSCRIPTION sub_nocols CONNECTION '$publisher_connstr' PUBLICATION pub_nocols WITH (binary = $binary);" + ); + + # Test initial sync. + $node_subscriber->wait_for_subscription_sync($node_publisher, 'sub_nocols'); + + is( $node_subscriber->safe_psql( + 'postgres', q{ + SELECT count(*) FROM tab_nocols + WHERE local_value = 'subscriber-default'; + }), + '2', + "initial zero-column rows use local defaults (binary = $binary)"); + + # Test incremental replication. + $node_publisher->safe_psql('postgres', + "INSERT INTO tab_nocols DEFAULT VALUES;"); + + $node_publisher->wait_for_catchup('sub_nocols'); + + is( $node_subscriber->safe_psql( + 'postgres', q{ + SELECT count(*) FROM tab_nocols + WHERE local_value = 'subscriber-default'; + }), + '3', + "incremental zero-column rows use local defaults (binary = $binary)"); + + # Drop the subscription before resetting both tables for the next mode. + $node_subscriber->safe_psql( + 'postgres', q{ + DROP SUBSCRIPTION sub_nocols; + TRUNCATE tab_nocols; + }); + $node_publisher->safe_psql('postgres', 'TRUNCATE tab_nocols;'); +} + +# Clean up before creating the FOR ALL TABLES publication. +$node_publisher->safe_psql( + 'postgres', q{ + DROP PUBLICATION pub_nocols; + DROP TABLE tab_nocols; +}); +$node_subscriber->safe_psql('postgres', 'DROP TABLE tab_nocols;'); + $node_publisher->safe_psql('postgres', "CREATE PUBLICATION tap_pub FOR ALL TABLES"); base-commit: 77705d1e5a616339c3e92f586bbfa7b6bb562beb
