Re: Proposal: Conflict log history table for Logical Replication

On Thu, 7 May 2026 at 14:29, vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> On Wed, 6 May 2026 at 19:35, Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> >
> > On Wed, May 6, 2026 at 6:28 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> > >
> > > On Wed, May 6, 2026 at 4:31 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > > >
> > > > On Wed, May 6, 2026 at 3:01 PM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
> > > > >
> > > > > On Wed, May 6, 2026 at 9:24 AM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> > > > > >
> > > > > > Few comments:
> > > > > > 1) Currently we allow renaming of pg_conflict schema, this might be ok
> > > > > > as we allow other sysem schema like pg_catalog and pg_toast also.
> > > > > > postgres=# alter schema pg_conflict rename to test_conflict;
> > > > > > ALTER SCHEMA
> > > > > >
> > > > >
> > > > > I agree that we allow renaming other schemas including pg_toast, but I
> > > > > am not sure if this is consciously made decision, see BUG #18281 ast
> > > > > [1]. I don't favour allowing renaming pg_conflict for 2 reasons:
> > > > >
> > > > > 1) Because Postgres explicitly blocks renaming schemas to a name
> > > > > starting with 'pg_'. If an admin accidentally renames 'pg_conflict' to
> > > > > something else, they are permanently locked out from renaming it back.
> > > > >
> > > > > 2) While the core worker might survive a rename via OID lookups;
> > > > > external scripts, extensions, and monitoring tools will likely
> > > > > hardcode the 'pg_conflict' string. If the schema is renamed, these
> > > > > tools will fail.
> > > > >
> > > >
> > > > I think we shouldn't go out of our way to disallow superusers to
> > > > rename pg_conflict schema similar to other cases. We can try to
> > > > prevent hard-coding schema names where possible but not sure we can
> > > > guarantee that nothing related to pg_conflict schema won't break as
> > > > shown by you in the following similar case for pg_conflict.
> > > >
> > > > > One such example of scripts breaking is present event in Postgres. I
> > > > > did the following, and most of psql commands started failing after
> > > > > that due to hard-coded pg_catalog name in them.
> > > > >
> > > > > postgres=# alter schema pg_catalog rename to catalog_new;
> > > > > ALTER SCHEMA
> > > > >
> > > > > postgres=# \d catalog_new.*
> > > > > ERROR: relation "pg_catalog.pg_class" does not exist
> > > > > LINE 5: FROM pg_catalog.pg_class c
> > > > >
> > > > > [1]: https://www.postgresql.org/message-id/flat/18281-5b1b6c5991d345aa%40postgresql.org
> > >
> > > I can see pg_toast and pg_catalog schema also hard coded in couple of
> > > places e.g.
> > >
> > > listPartitionedTables()
> > > {
> > > if (!pattern)
> > > appendPQExpBufferStr(&buf, " AND n.nspname <> 'pg_catalog'\n"
> > > " AND n.nspname !~ '^pg_toast'\n"
> > > " AND n.nspname <> 'information_schema'\n");
> > > }
> > >
> > > I will analyze which all places we are hardcoding, I think on server
> > > side code we can easily avoid but from client side e.g. describe we
> > > might need to invent a way to identify the schema name, or we might
> > > have to store it somewhere in pg_subscription etc, I don't think we
> > > should go that route.
> >
> > Here is updated patch set
>
> Thanks for the updated patches, the v30 version patch posted has few issues:
> There is an assert at [1]:
> TRAP: failed Assert("conflictlogrel != NULL"), File:
> "../src/backend/replication/logical/conflict.c", Line: 195, PID: 59658
> 0xb3a472 <ExceptionalCondition+0x82> at
> /tmp/cirrus-ci-build/build/tmp_install/usr/local/pgsql/bin/postgres
> 0x9433b8 <ReportApplyConflict+0x13c8> at
> /tmp/cirrus-ci-build/build/tmp_install/usr/local/pgsql/bin/postgres
> 0x7d91fb <CheckAndReportConflict+0x2cb> at
> /tmp/cirrus-ci-build/build/tmp_install/usr/local/pgsql/bin/postgres
> 0x7d8b4b <ExecSimpleRelationInsert+0x10b> at
> /tmp/cirrus-ci-build/build/tmp_install/usr/local/pgsql/bin/postgres
> 0x96525b <apply_dispatch+0x23eb> at
> /tmp/cirrus-ci-build/build/tmp_install/usr/local/pgsql/bin/postgres
> 0x966150 <start_apply+0x310> at
> /tmp/cirrus-ci-build/build/tmp_install/usr/local/pgsql/bin/postgres
> 0x967010 <run_apply_worker+0x290> at
> /tmp/cirrus-ci-build/build/tmp_install/usr/local/pgsql/bin/postgres
> 0x966d6d <ApplyWorkerMain+0x1d> at
> /tmp/cirrus-ci-build/build/tmp_install/usr/local/pgsql/bin/postgres
> 0x90ff0c <BackgroundWorkerMain+0x1cc> at
> /tmp/cirrus-ci-build/build/tmp_install/usr/local/pgsql/bin/postgres
> 0x914a25 <postmaster_child_launch+0x145> at
> /tmp/cirrus-ci-build/build/tmp_install/usr/local/pgsql/bin/postgres
> 0x917b77 <maybe_start_bgworkers+0x1d7> at
> /tmp/cirrus-ci-build/build/tmp_install/usr/local/pgsql/bin/postgres
> 0x9198f5 <ServerLoop+0x1c65> at
> /tmp/cirrus-ci-build/build/tmp_install/usr/local/pgsql/bin/postgres
> 0x917156 <PostmasterMain+0x1116> at
> /tmp/cirrus-ci-build/build/tmp_install/usr/local/pgsql/bin/postgres
> 0x83c16d <main+0x48d> at
> /tmp/cirrus-ci-build/build/tmp_install/usr/local/pgsql/bin/postgres
>
> There are the following warnings at [2]:
> [14:55:07.472] conflict.c:187:6: error: variable 'log_dest_clt' is
> used uninitialized whenever 'if' condition is false
> [-Werror,-Wsometimes-uninitialized]
> [14:55:07.472] 187 | if (dest == CONFLICT_LOG_DEST_TABLE ||
> dest == CONFLICT_LOG_DEST_ALL)
> [14:55:07.472] |
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> [14:55:07.472] conflict.c:193:6: note: uninitialized use occurs here
> [14:55:07.472] 193 | if (log_dest_clt)
> [14:55:07.472] | ^~~~~~~~~~~~
> [14:55:07.472] conflict.c:187:2: note: remove the 'if' if its
> condition is always true
> [14:55:07.472] 187 | if (dest == CONFLICT_LOG_DEST_TABLE ||
> dest == CONFLICT_LOG_DEST_ALL)
> [14:55:07.472] |
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> [14:55:07.472] 188 | log_dest_clt = true;
> [14:55:07.472] conflict.c:176:21: note: initialize the variable
> 'log_dest_clt' to silence this warning
> [14:55:07.472] 176 | bool log_dest_clt;
> [14:55:07.472] | ^
> [14:55:07.472] | = false
> [14:55:07.472] 1 error generated.
>
> [1] - https://api.cirrus-ci.com/v1/artifact/task/5630092254117888/testrun/build/testrun/subscription/026_stats/log/026_stats_subscriber.log
> [2] - https://cirrus-ci.com/task/5770829742473216

In the below, log_dest_clt is declared without initialization. Later,
they are assigned only for specific dest values. This leaves a bug
when dest is set to CONFLICT_LOG_DEST_LOG. In that case,log_dest_clt
retains an indeterminate stack value. Because log_dest_clt is
uninitialized, it may evaluate to true depending on the garbage value
present on the stack. That can incorrectly enter the CLT insertion
path and trigger assertion failure.

...
@@ -131,30 +170,92 @@ ReportApplyConflict(EState *estate,
ResultRelInfo *relinfo, int elevel,
ConflictType type,
TupleTableSlot *searchslot,
TupleTableSlot *remoteslot,
List *conflicttuples)
{
- Relation localrel = relinfo->ri_RelationDesc;
- StringInfoData err_detail;
+ Relation localrel = relinfo->ri_RelationDesc;
+ ConflictLogDest dest;
+ Relation conflictlogrel;
+ bool log_dest_clt;
+ bool log_dest_logfile;
...

...
- pgstat_report_subscription_conflict(MySubscription->oid, type);
+ if (dest == CONFLICT_LOG_DEST_TABLE || dest == CONFLICT_LOG_DEST_ALL)
+ log_dest_clt = true;
+ if (dest == CONFLICT_LOG_DEST_LOG || dest == CONFLICT_LOG_DEST_ALL)
+ log_dest_logfile = true;

- ereport(elevel,
- errcode_apply_conflict(type),
- errmsg("conflict detected on relation
\"%s.%s\": conflict=%s",
-
get_namespace_name(RelationGetNamespace(localrel)),
- RelationGetRelationName(localrel),
- ConflictTypeNames[type]),
- errdetail_internal("%s", err_detail.data));
+ /* Insert to table if requested. */
+ if (log_dest_clt)
+ {
+ Assert(conflictlogrel != NULL);
...

The attached v31 version has the changes to fix this issue by
initializing the variable.
This also has the rebased version along with the rebased version of
the 'Preserve conflict log destination and subscription OID for
subscriptions' patch which is present in the 0005 patch.

Regards,
Vignesh