Re: Proposal: Conflict log history table for Logical Replication

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?

Thanks,
Shlok Kyal