From a498150701e577e5f758452488321b44e7f279ff Mon Sep 17 00:00:00 2001 From: Nisha Moond Date: Fri, 16 Oct 2026 19:12:12 +0530 Subject: [PATCH v82 3/3] Address review comments on v81-0001 - Take replica_identity key names from the table instead of the index, whose column names can be stale (e.g. after a column rename) or wrong (e.g. UNIQUE (a, a)). - Build the replica identity JSON with fixed IntervalStyle and extra_float_digits, as already done for DateStyle, so the stored values convert back to the original keys. Add a test. - Assert that remote_xid is valid instead of storing NULL. - Explain in comments why HEAP_INSERT_NO_LOGICAL is needed (TOAST inserts) and why the oversize marker keeps no prefix. - Docs: explain that bytea_output and quote_all_identifiers change the recorded text, that local_conflicts origin is NULL when unset or dropped, and that remote_commit_ts is NULL for prepared transactions; drop the stale "not applicable" case from replica_identity. --- doc/src/sgml/logical-replication.sgml | 32 ++++-- src/backend/replication/logical/conflict.c | 43 ++++++-- src/test/subscription/t/035_conflicts.pl | 109 +++++++++++++++++++++ 3 files changed, 171 insertions(+), 13 deletions(-) diff --git a/doc/src/sgml/logical-replication.sgml b/doc/src/sgml/logical-replication.sgml index 0d88cbfd4fd..8bc0e9e2e9c 100644 --- a/doc/src/sgml/logical-replication.sgml +++ b/doc/src/sgml/logical-replication.sgml @@ -2361,7 +2361,7 @@ DETAIL: detailed_explanation[: remote_commit_ts timestamptz - The remote commit timestamp of the remote transaction. This is NULL if the conflict is handled by a parallel apply worker, because in-progress transactions are streamed and applied before the remote commit timestamp is known. + The remote commit timestamp of the remote transaction. This is NULL if the conflict is handled by a parallel apply worker, because in-progress transactions are streamed and applied before the remote commit timestamp is known. It is also NULL if the conflict occurs in a transaction that is prepared on the subscriber (see two_phase), because the commit timestamp is known only when COMMIT PREPARED arrives. remote_origin @@ -2379,8 +2379,7 @@ DETAIL: detailed_explanation[: The replica identity key values identifying the conflicting row, as a JSON object keyed by column name; see below for how the values are represented. This is NULL when - replica_identity_full is true or - when replica identity is not applicable. + replica_identity_full is true. local_conflicts @@ -2389,9 +2388,13 @@ DETAIL: detailed_explanation[: xid), commit timestamp - (commit_ts), and origin (origin). - This is NULL if there are no local conflicting rows - (e.g., in update_missing and + (commit_ts), and the name of the replication + origin that last modified the row (origin). + The origin field is NULL if + the transaction had no replication origin set or if that origin has + since been dropped. The column itself is NULL + if there are no local conflicting rows (e.g., in + update_missing and delete_missing conflicts). @@ -2441,6 +2444,23 @@ SELECT (replica_identity->>'doc')::jsonb->>'x' FROM ...; -- 1 + + The apply worker's and + settings change how a value + is written without changing the value itself. If these settings differ + between conflicts, the same key can be recorded in different but + equivalent forms, and comparing the recorded text may miss some of them. + To match conflicts on the same key, cast the value back to the column's + type instead, here for a bytea key column b + and a regclass[] key column r: + +SELECT * FROM ... WHERE replica_identity->>'b' = '\x0102'; -- may miss rows +SELECT * FROM ... WHERE (replica_identity->>'b')::bytea = '\x0102'; -- matches all +SELECT * FROM ... WHERE replica_identity->>'r' = '{t}'; -- may miss rows +SELECT * FROM ... WHERE (replica_identity->>'r')::regclass[] = '{t}'; -- matches all + + + If a key value's text representation is larger than 64kB, it is not recorded. In its place the conflict log stores a small object marking it diff --git a/src/backend/replication/logical/conflict.c b/src/backend/replication/logical/conflict.c index b6ed0847475..a2037ba716a 100644 --- a/src/backend/replication/logical/conflict.c +++ b/src/backend/replication/logical/conflict.c @@ -1069,7 +1069,7 @@ build_index_key_json(EState *estate, Relation localrel, bool *omitted) { Relation indexDesc; - TupleDesc indexTupDesc; + TupleDesc reltupdesc = RelationGetDescr(localrel); Datum values[INDEX_MAX_KEYS]; bool isnull[INDEX_MAX_KEYS]; StringInfoData result; @@ -1082,15 +1082,26 @@ build_index_key_json(EState *estate, Relation localrel, Assert(CheckRelationOidLockedByMe(indexid, RowExclusiveLock, true)); indexDesc = index_open(indexid, NoLock); - indexTupDesc = RelationGetDescr(indexDesc); build_index_datums_from_slot(estate, localrel, slot, indexDesc, values, isnull); + /* + * Store the key values in a consistent, standard format, independent of + * the apply worker's DateStyle, IntervalStyle and extra_float_digits + * settings, so that the stored key values always convert back + * unambiguously to the original ones. + */ save_nestlevel = NewGUCNestLevel(); (void) set_config_option("datestyle", "ISO", PGC_USERSET, PGC_S_SESSION, GUC_ACTION_SAVE, true, 0, false); + (void) set_config_option("intervalstyle", "postgres", + PGC_USERSET, PGC_S_SESSION, + GUC_ACTION_SAVE, true, 0, false); + (void) set_config_option("extra_float_digits", "3", + PGC_USERSET, PGC_S_SESSION, + GUC_ACTION_SAVE, true, 0, false); /* * Build the JSON object here rather than handing the values to @@ -1118,7 +1129,11 @@ build_index_key_json(EState *estate, Relation localrel, for (int i = 0; i < indnkeyatts; i++) { - Form_pg_attribute att = TupleDescAttr(indexTupDesc, i); + Form_pg_attribute att; + AttrNumber attnum = indexDesc->rd_index->indkey.values[i]; + + Assert(attnum > 0); /* RI and PK indexes have no expressions */ + att = TupleDescAttr(reltupdesc, attnum - 1); if (i > 0) appendStringInfoChar(&result, ','); @@ -1146,6 +1161,12 @@ build_index_key_json(EState *estate, Relation localrel, /* * Don't record a value larger than CONFLICT_MAX_VALUE_SIZE; put a * marker recording its length in its place. + * + * The marker deliberately keeps no prefix of the value. For + * arrays, composites and similar types the leading bytes are + * mostly punctuation and say little about the value. Keeping a + * prefix would also require rendering every oversized value, + * ruling out skipping one based on its stored size alone. */ if (outputlen > CONFLICT_MAX_VALUE_SIZE) { @@ -1350,10 +1371,13 @@ insert_conflict_log_tuple(EState *estate, Relation rel, values[attno++] = CStringGetTextDatum(ConflictTypeNames[conflict_type]); - if (TransactionIdIsValid(remote_xid)) - values[attno++] = TransactionIdGetDatum(remote_xid); - else - nulls[attno++] = true; + /* + * The remote xid is always valid here: a conflict is only detected while + * applying a row change, which is preceded by a BEGIN, BEGIN PREPARE or + * STREAM START that recorded a valid top-level xid. + */ + Assert(TransactionIdIsValid(remote_xid)); + values[attno++] = TransactionIdGetDatum(remote_xid); if (XLogRecPtrIsValid(remote_final_lsn)) values[attno++] = LSNGetDatum(remote_final_lsn); @@ -1417,6 +1441,11 @@ insert_conflict_log_tuple(EState *estate, Relation rel, tuple = heap_form_tuple(RelationGetDescr(conflictlogrel), values, nulls); + /* + * The conflict log table itself is never logically decoded, but its TOAST + * table would be. heap_insert() passes HEAP_INSERT_NO_LOGICAL on to the + * TOAST inserts, so toasted values are not decoded either. + */ heap_insert(conflictlogrel, tuple, GetCurrentCommandId(true), HEAP_INSERT_NO_LOGICAL, NULL); diff --git a/src/test/subscription/t/035_conflicts.pl b/src/test/subscription/t/035_conflicts.pl index 2418d4f19c6..b47a74208a0 100644 --- a/src/test/subscription/t/035_conflicts.pl +++ b/src/test/subscription/t/035_conflicts.pl @@ -166,6 +166,115 @@ $node_subscriber->wait_for_log( pass('multiple_unique_conflicts detected on a leaf partition during insert'); +############################################################################### +# Verify that the JSON stored in the conflict log table uses a consistent, +# standard format, independent of the apply worker's DateStyle / IntervalStyle / +# extra_float_digits settings, so that the stored key values always convert +# back to the original ones. +############################################################################### + +# Clear the error state left by the leaf partition test. +$node_subscriber->safe_psql('postgres', "TRUNCATE conf_tab_2;"); +$node_publisher->wait_for_catchup($appname); + +# Non-default output-related GUCs for sessions of the test database; the apply +# worker picks them up when it (re)connects. +$node_subscriber->safe_psql('postgres', + "ALTER DATABASE postgres SET IntervalStyle = 'sql_standard'"); +$node_subscriber->safe_psql('postgres', + "ALTER DATABASE postgres SET extra_float_digits = -2"); +$node_subscriber->safe_psql('postgres', + "ALTER DATABASE postgres SET DateStyle = 'SQL, MDY'"); + +# Table with interval/float8/timestamptz replica identity key columns. +my $guc_ddl = qq( + CREATE TABLE conf_guc (a int, i interval NOT NULL, f float8 NOT NULL, + ts timestamptz NOT NULL); + CREATE UNIQUE INDEX conf_guc_ri ON conf_guc (i, f, ts); + ALTER TABLE conf_guc REPLICA IDENTITY USING INDEX conf_guc_ri; +); +$node_publisher->safe_psql('postgres', $guc_ddl); +$node_subscriber->safe_psql('postgres', $guc_ddl); + +$node_publisher->safe_psql('postgres', + "ALTER PUBLICATION pub_tab ADD TABLE conf_guc"); +$node_publisher->safe_psql( + 'postgres', + "INSERT INTO conf_guc VALUES (1, '-1 day -11:22:33', 1.23456789012345, + '2026-03-04 10:11:12.123456+05:30')"); + +# Pick up the new table, and restart the apply worker so that it picks up the +# new database settings. +$node_subscriber->safe_psql('postgres', + "ALTER SUBSCRIPTION sub_tab REFRESH PUBLICATION"); +$node_subscriber->safe_psql('postgres', "ALTER SUBSCRIPTION sub_tab DISABLE"); + +# Wait for the apply worker to stop +$node_subscriber->poll_query_until('postgres', + "SELECT count(*) = 0 FROM pg_stat_activity WHERE backend_type = 'logical replication apply worker'" +); + +$node_subscriber->safe_psql('postgres', "ALTER SUBSCRIPTION sub_tab ENABLE"); +$node_subscriber->wait_for_subscription_sync($node_publisher, $appname); + +# Generate an update_missing conflict; its replica_identity JSON in the +# conflict log table contains the key values. +$node_subscriber->safe_psql('postgres', "DELETE FROM conf_guc"); +$node_publisher->safe_psql('postgres', + "UPDATE conf_guc SET a = 2 WHERE a = 1"); + +$node_subscriber->poll_query_until( + 'postgres', + "SELECT count(*) > 0 FROM $clt WHERE relname = 'conf_guc' + AND conflict_type = 'update_missing'" +) or die "timed out waiting for conf_guc conflict row"; + +# The stored interval and float formats must be the standard ones, so they +# can be converted back to the original values even when the apply worker +# ran with IntervalStyle = sql_standard and extra_float_digits = -2. +is( $node_subscriber->safe_psql( + 'postgres', + "SELECT replica_identity->>'i', replica_identity->>'f' + FROM $clt WHERE relname = 'conf_guc'"), + '-1 days -11:22:33|1.23456789012345', + 'CLT JSON stores canonical interval and lossless float format'); + +# The timestamp format is ISO (DateStyle-independent) even with +# DateStyle = 'SQL, MDY'. +like( + $node_subscriber->safe_psql( + 'postgres', + "SELECT replica_identity->>'ts' FROM $clt WHERE relname = 'conf_guc'" + ), + qr/^\d{4}-\d{2}-\d{2} \d{2}:\d{2}:\d{2}(\.\d+)?[+-]\d{2}(:\d{2}){0,2}$/, + 'CLT JSON stores timestamp in ISO format'); + +# A reader with default settings must get the original key values back. +is( $node_subscriber->safe_psql( + 'postgres', + "SET IntervalStyle = 'postgres'; SET extra_float_digits = 1; + SET DateStyle = 'SQL, DMY'; + SELECT (replica_identity->>'i')::interval + = interval '-1 day -11:22:33', + (replica_identity->>'f')::float8 + = 1.23456789012345::float8, + (replica_identity->>'ts')::timestamptz + = timestamptz '2026-03-04 10:11:12.123456+05:30' + FROM $clt WHERE relname = 'conf_guc'"), + 't|t|t', + 'CLT JSON values convert back to the original key values'); + +# Restore the defaults for any later tests. +$node_subscriber->safe_psql('postgres', + "ALTER DATABASE postgres RESET IntervalStyle"); +$node_subscriber->safe_psql('postgres', + "ALTER DATABASE postgres RESET extra_float_digits"); +$node_subscriber->safe_psql('postgres', + "ALTER DATABASE postgres RESET DateStyle"); + +# Disable it so that later waits for all apply workers to stop don't hang. +$node_subscriber->safe_psql('postgres', "ALTER SUBSCRIPTION sub_tab DISABLE"); + ############################################################################### # Setup a bidirectional logical replication between node_A & node_B ############################################################################### -- 2.54.0 (Apple Git-157)