Re: Proposal: Conflict log history table for Logical Replication

On Thu, 18 Jun 2026 at 16:54, Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> On Thu, Jun 18, 2026 at 11:29 AM Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> wrote:
> >
> > On Wed, 17 Jun 2026 at 18:31, vignesh C <vignesh21(at)gmail(dot)com> wrote:
> > >
> > > On Tue, 16 Jun 2026 at 05:19, Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> > > >
> > > > On Mon, Jun 8, 2026 at 9:39 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> > > > >
> > > > > On Fri, 5 Jun 2026 at 07:59, Peter Smith <smithpb2250(at)gmail(dot)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.

FYI, the patch needs a rebase because of the recent commits:
Applying: Add configurable conflict log table for Logical Replication
error: patch failed: src/backend/commands/subscriptioncmds.c:2260
error: src/backend/commands/subscriptioncmds.c: patch does not apply

Regards,
Vignesh