Re: Proposal: Conflict log history table for Logical Replication - Mailing list pgsql-hackers

On Thu, 18 Jun 2026 at 16:54, Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Thu, Jun 18, 2026 at 11:29 AM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
> >
> > On Wed, 17 Jun 2026 at 18:31, vignesh C <vignesh21@gmail.com> wrote:
> > >
> > > On Tue, 16 Jun 2026 at 05:19, Peter Smith <smithpb2250@gmail.com> wrote:
> > > >
> > > > On Mon, Jun 8, 2026 at 9:39 PM vignesh C <vignesh21@gmail.com> wrote:
> > > > >
> > > > > On Fri, 5 Jun 2026 at 07:59, Peter Smith <smithpb2250@gmail.com> wrote:
> > > > > >
> > > > > > Hi Vignesh.
> > > > > >
> > > > > > Some review comments for the patch v45-0004.
> > > > > >
> > > > > > 4c.
> > > > > > IMO there should be a separate function for handling the subscription
> > > > > > footer/s, same as there is already a function
> > > > > > addFooterToPublicationDesc.
> > > > >
> > > > > It is not required in this case as we don't have multiple footers from
> > > > > different places to be added here.
> > > > >
> > > >
> > > > Sure, it's not "required", but I think:
> > > > A) Separating the footer code from the non-footer code makes it easier to read
> > > > B) The 'describeSubscriptions' function is too long. This would make
> > > > it 20 lines shorter.
> > > > C) Consistent footer handling for pub/sub describes.
> > >
> > > Ok, Let's keep it consistent
> > >
> > > > //////
> > > >
> > > > More review comments for v50-0005
> > > >
> > > > ======
> > > > src/bin/psql/describe.c
> > > >
> > > > 1.
> > > > + /* Conflict log destination is supported in v19 and higher */
> > > > + if (pset.sversion >= 190000)
> > > >
> > > > The CLT is targeting PG20, right? So, that comment ought to say "is
> > > > supported in v20 and higher".
> > > >
> > > > Ideally, there should be some "TODO" reminder comments here to ensure
> > > > the appropriate 190000's get replaced by 200000 as soon as the version
> > > > number is bumped. Better to flag/comment all those places now, so that
> > > > nothing gets missed later.
> > > >
> > > > (A similar review comment probably applies also to the pg_dump changes
> > > > in the previous v50-0004 patch).
> > >
> > > I felt it is better to mention it in the commit message of the patch
> > > instead of mentioning it in the code.
> > >
> > > The attached v51 version patch has the changes for the same.
> > > The patch also addresses the comment from [1].
> > > [1] -
https://www.postgresql.org/message-id/CANhcyEUjc9TCcW1YAQVMTs6-huWBZoy%2BsVkz5C8b72os5p-f%2Bg%40mail.gmail.com
> > >
> > Hi Dilip/ Vignesh,
> >
> > I reviewed the patch and here are some comments:
> >
> > 1. I further tested for the object which can be created in 'pg_conflict' schemas
> > and found that operations such CREATE EXTENSION, CREATE OPERATOR,
> > CREATE COLLATION, CREATE TEXT SEARCH DICTIONARY on the schema
> > pg_conflict.
> > Is this expected?
> >
> > postgres=# CREATE EXTENSION hstore WITH SCHEMA pg_conflict;
> > CREATE EXTENSION
> > postgres=# CREATE EXTENSION pg_walinspect WITH SCHEMA pg_conflict;
> > CREATE EXTENSION
> > postgres=# CREATE COLLATION pg_conflict.mycollation (provider =
> > libc,locale = 'C');
> > CREATE COLLATION
> > postgres=# CREATE TEXT SEARCH DICTIONARY pg_conflict.simple_dict
> > (TEMPLATE = pg_catalog.simple);
> > CREATE TEXT SEARCH DICTIONARY
> > postgres=# CREATE OPERATOR pg_conflict.=== (LEFTARG = int, RIGHTARG =
> > int, PROCEDURE = int4eq);
> > CREATE OPERATOR
> >
> > Also, when we create extension several objects are created in the schema:
> > eg:
> >  extname |                                                   object
> >
---------+------------------------------------------------------------------------------------------------------------
> >  hstore  | type pg_conflict.hstore
> >  hstore  | function pg_conflict.hstore_in(cstring)
> >  hstore  | function pg_conflict.hstore_out(pg_conflict.hstore)
> >  hstore  | function pg_conflict.hstore_recv(internal)
> >  hstore  | function pg_conflict.hstore_send(pg_conflict.hstore)
> >  hstore  | type pg_conflict.hstore[]
> >  hstore  | function pg_conflict.hstore_version_diag(pg_conflict.hstore)
> >  .
> >  .
> >
> > So, should it be allowed?
> >
> > 2. When we try to DROP conflict log table we get an ERROR:
> > postgres=# DROP TABLE pg_conflict.pg_conflict_log_16395;
> > ERROR:  permission denied: "pg_conflict_log_16395" is a system catalog
> >
> > But for other restricted operation such as UPDATE/INSERT we get the error as:
> > postgres=# INSERT INTO pg_conflict.pg_conflict_log_16395 VALUES (1);
> > ERROR:  cannot modify or insert data into conflict log table
> > "pg_conflict_log_16395"
> > DETAIL:  Conflict log tables are system-managed and only support
> > cleanup via DELETE or TRUNCATE.
> >
> > I think for the DROP case we should throw a similar error. Thoughts?
>
> Please find the updated patch version (rebased on current HEAD), which
> fixes all the open comments in 0001 and 0002. We are still working on
> 0003 regarding handling conflict log table insertion errors.

While attempting to log a conflict, a concurrent ALTER SUBSCRIPTION
can change the conflict logging destination from all to log. In this
scenario, the apply worker may already have cached the conflictlogdest
information, including the OID of the current conflict log table.
However, the concurrent ALTER SUBSCRIPTION drops the conflict log
table as part of the destination change:
+Relation
+GetConflictLogDestAndTable(ConflictLogDest *log_dest)
+{
+       Oid                     conflictlogrelid;
+
+       /*
+        * Convert the text log destination to the internal enum.
MySubscription
+        * already contains the data from pg_subscription.
+        */
+       *log_dest = GetConflictLogDest(MySubscription->conflictlogdest);
+
+       /* Quick exit if a conflict log table was not requested. */
+       if (!CONFLICTS_LOGGED_TO_TABLE(*log_dest))
+               return NULL;
+
+       conflictlogrelid = MySubscription->conflictlogrelid;
+
+       Assert(OidIsValid(conflictlogrelid));
+
+       return table_open(conflictlogrelid, RowExclusiveLock);
+}

As a result, when the apply worker later attempts to open the cached
conflict log table, table_open() fails because the relation has
already been dropped. This causes the error handling path itself to
fail before the conflict record can be written to either the conflict
log table or the server log.

In such cases, the conflict record is effectively lost and is not
logged anywhere. For example:
2026-06-21 19:31:13.592 IST [263598] LOG:  logical replication apply
worker for subscription "sub1" has started
2026-06-21 19:32:26.731 IST [263598] ERROR:  could not open relation
with OID 16405
2026-06-21 19:32:26.731 IST [263598] CONTEXT:  processing remote data
for replication origin "pg_16404" during message type "INSERT" for
replication target relation "public.t1" in transaction 698, finished
at 0/017D39A0
2026-06-21 19:32:26.735 IST [263471] LOG:  background worker "logical
replication apply worker" (PID 263598) exited with exit code 1

Ideally, failure to access the conflict log table should not prevent
the conflict from being reported in the server log. This issue is
present with the v52 version. I have not yet checked if Amit's recent
patch posted a few minutes ago at [1] handles this issue.
Thoughts?

[1] - https://www.postgresql.org/message-id/CAA4eK1JhZYRMP_YYa1j3uAK6L4v057JDuM0%2BYLABOgAOYuwM8Q%40mail.gmail.com

Regards,
Vignesh