On Fri, Jun 5, 2026 at 4:22 PM Dilip Kumar <[email protected]> wrote:
>
> Here is the updated patch which fixes all open issues except Peter
> reported on 0004 patch, Vignesh would you take care of that?
>
Thanks Dilip for the updated patches.
Patch 001 + 002
1) Issue #5 from [1] still appears to be present. I still see NULL
values in remote_commit_ts. From patch, it looks like remote_commit_ts
is updated in v46, but too late in the flow. I tested by moving:
/* Set remote_commit_ts for conflict logging. */
remote_commit_ts = commit_data.committime;
before the apply_spooled_messages() call, and remote_commit_ts was
logged correctly. It seems the assignment needs to happen earlier.
2) CLT reports NULL or stale remote_xid for two-phase transactions
Test case:
setup : pub-sub nodes with max_prepared_transactions = 10 to enable two_phase
-- Publisher setup:
CREATE TABLE clt_test (id int PRIMARY KEY);
CREATE PUBLICATION clt_pub FOR TABLE clt_test;
-- Subscriber setup:
CREATE TABLE clt_test (id int PRIMARY KEY);
INSERT INTO clt_test VALUES (1); -- pre-existing row to conflict
CREATE SUBSCRIPTION clt_sub
CONNECTION 'port=9933 dbname=postgres'
PUBLICATION clt_pub
WITH (two_phase = true, conflict_log_destination = 'all');
-- Trigger a conflict via a prepared transaction on the publisher:
-- Publisher:
BEGIN;
select txid_current();
INSERT INTO clt_test VALUES (1); -- will conflict on subscriber
PREPARE TRANSACTION 'clt_bug_test';
COMMIT PREPARED 'clt_bug_test';
-- Check the CLT on the subscriber. The publisher transaction was 699,
but the CLT contains:
postgres=# SELECT conflict_type, remote_xid, remote_commit_ts FROM
pg_conflict.pg_conflict_log_16398 ;
conflict_type | remote_xid | remote_commit_ts
---------------+------------+----------------------------------
insert_exists | 698 | 2026-06-08 14:04:34.903284+05:30
insert_exists | |
insert_exists | |
...
It looks like remote_xid is either stale values from a previous
transaction or remain unset when apply worker tries again.
The change below fixes it.
@@ -1300,6 +1300,8 @@ apply_handle_begin_prepare(StringInfo s)
set_apply_error_context_xact(begin_data.xid, begin_data.prepare_lsn);
remote_final_lsn = begin_data.prepare_lsn;
+ remote_xid = begin_data.xid;
+ remote_commit_ts = 0;
~~~
patch-003
3) alter_sub_conflictlogdestination():
+ relid = get_relname_relid(relname, PG_CONFLICT_NAMESPACE);
+ if (OidIsValid(relid))
+ {
+ /* Existing table from upgrade */
+ Assert(IsBinaryUpgrade);
+ }
+ else
+ {
+ relid = create_conflict_log_table(sub->oid, sub->name,
+ sub->owner);
+ }
+
I think Assert() above should be replaced with an ERROR similar to the
one in create_conflict_log_table().
One possible scenario is that a user manually creates
pg_conflict.pg_conflict_log_<subid> with allow_system_table_mods = on.
In that case:
-- Assert builds will hit the assertion.
-- Non-assert builds will silently skip table creation, and later CLT
operations may fail in unexpected ways because the user-created table
definition may not match the expected schema.
Before patch-003, create_conflict_log_table() correctly reported an
error if a table with the same name already existed, which seems
safer.
4) 002_pg_dump.pl
- WITH (connect = false, origin = any, streaming = on);',
+ WITH (connect = false, origin = any, streaming = on,
conflict_log_destination= table);',
typo: space before "=".
/conflict_log_destination= table/conflict_log_destination = table/
~~~
patch-005
5) doc/src/sgml/ddl.sgml
+ direct user manipulation. Unlike <literal>pg_catalog</literal>, the
+ <literal>pg_catalog</literal> schema is not implicitly included in the
+ search path, so objects within it must be referenced explicitly or by
typo : repeated "pg_catalog"
It should be: Unlike <literal>pg_catalog</literal>, the
<literal>pg_conflict</literal> schema is not implicitly...
~~~
[1]
https://www.postgresql.org/message-id/CABdArM5N0LgkHc%2BJOKcbDjpx%3D0hdWDx%2BJ7y%3DUS2apEEmi07oyw%40mail.gmail.com
--
Thanks,
Nisha
