On Mon, Jun 15, 2026 at 4:45 PM Dilip Kumar <[email protected]> wrote:
>
> 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.
> */
Works for me.
>
> > 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,
This is because we compute want_table and has_oldtable purely based on
old and new settings (not looking at table existence actually), so the
old setting is !has_oldtable and new is want_table, thus hitting the
Assert. IMO, it should be ERROR (similar to create_conflict_log_table)
rather than Assert if the table exists and the mode is not
BinaryUpgrade.
Thanks
Shveta
