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? Thanks, Shlok Kyal
