On Mon, Jun 15, 2026 at 11:27 AM shveta malik <[email protected]> wrote:
>
> A few comments on v50:
>
>
> 1)
> + /*
> + * Conflict log tables are used internally for logical replication
> + * conflict logging and should not have extended statistics, as it
> + * could disrupt conflict logging.
> + */
>
> Suggestion:
> /*
> * Conflict log tables are system-managed tables used internally for
> * logical replication conflict logging. Similar to indexes, user-defined
> * extended statistics are not supported on these tables at present.
> */
I think the reason for index are different so here is my proposal for this
/*
* Conflict log tables are system-managed tables used internally for
* logical replication conflict logging. Unlike user tables, they are
* not expected to have complex query usage, so to keep things simple,
* user-defined extended statistics are not required or supported at
* present.
*/
> 2)
> Assertion hit in alter_sub_conflict_log_dest():
>
>
> set allow_system_table_mods=on
> create subscription sub1 connection '...' publication pub1
> WITH(conflict_log_destination='log');
> select oid from pg_subscription;
>
> --CREATE clt USING SAME oid
> create table pg_conflict.pg_conflict_log_16501(i int);
>
> --change destination to table/all
> alter subscription sub1 SET (CONFLICT_LOG_DESTINATION = 'ALL');
>
> --this asserts here:
> 2026-06-15 11:10:40.540 IST [10626] LOG: background worker "logical
> replication tablesync worker" (PID 11397) exited with exit code 1
> TRAP: failed Assert("IsBinaryUpgrade"), File: "subscriptioncmds.c",
> Line: 1554, PID: 10662
>
> alter_sub_conflict_log_dest(): Assert(IsBinaryUpgrade);
>
> Should we fix Assert or restrict table creation in pg_conflict scehma?
> We allow it for toast-schema though when allow_system_table_mods=ON.
> But I don't see a use case for allowing this.
I think this is in 0004 patch, because during upgrade we would have an
existing table created via schema dump, and hence we need to associate
this table to the subscription. But I am surprise to see that
changinng destination from 'log' to 'all' is going in that path where
table already exists, Vignesh can you debug this change, I think it
got introduced in 0004
> 3)
> I'm not sure whether the comments (at [1]) regarding the TRY-CATCH
> block were considered or perhaps overlooked. Please ignore this if you
> are already taking them into account.
Yeah all 0003 review commants are not yet fixed, there are few
comments from you as well as from Nisha, will fix in next version
>
> Doc-patch:
> <caught my eye while checking something, not reviewing the doc-patch
> rigth now though>
>
> 4)
> + 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
> + adjusting the search path.
>
>
> Second pg_catalog should be replaced with pg_conflict
>
> 5)
> + as the <emphasis>conflict schema</emphasis>) contains system managed
>
> + The <literal>pg_conflict</literal> schema that contains system-managed
>
> system managed or system-managed, we can use same at both the places.
>
> ~~
>
> [1]:
> https://www.postgresql.org/message-id/CAJpy0uBSY7zTH%3D4TvAOS%3Dkj9vivBUc9NO%2BVp6KNw-Na9RiAsMg%40mail.gmail.com
I am making a note so that we can do all doc changes later.
--
Regards,
Dilip Kumar
Google
