Re: Proposal: Conflict log history table for Logical Replication

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.

--
Regards,
Dilip Kumar
Google