On Thu, 18 Jun 2026 at 16:54, Dilip Kumar <[email protected]> wrote: > > On Thu, Jun 18, 2026 at 11:29 AM Shlok Kyal <[email protected]> wrote: > > > > On Wed, 17 Jun 2026 at 18:31, vignesh C <[email protected]> wrote: > > > > > > On Tue, 16 Jun 2026 at 05:19, Peter Smith <[email protected]> wrote: > > > > > > > > On Mon, Jun 8, 2026 at 9:39 PM vignesh C <[email protected]> wrote: > > > > > > > > > > On Fri, 5 Jun 2026 at 07:59, Peter Smith <[email protected]> > > > > > 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
