On Wed, 20 May 2026 at 15:05, vignesh C <[email protected]> wrote: > > On Tue, 19 May 2026 at 12:02, Peter Smith <[email protected]> wrote: > > > > On Mon, May 18, 2026 at 10:35 PM vignesh C <[email protected]> wrote: > > > > > > On Wed, 13 May 2026 at 11:43, Peter Smith <[email protected]> wrote: > > > > Hi Vignesh. > > > > Thanks for addressing lots of my previous v33-0001 review comments. > > > > Here are some more review comments for the combined v35-0001/0002 patches. > > > > ====== > > Commit message. > > > > 1. > > If the user chooses to enable logging to a table (by selecting 'table' > > or 'all'), > > an internal logging table named pg_conflict_log_<subid> is automatically > > created within a dedicated, system-managed 'pg_conflict' namespace to > > prevent > > users from manually dropping or altering it. This also prevents accidental > > name collisions with user-created tables. This table is linked to the > > subscription via an internal dependency, ensuring it is automatically > > dropped > > when the subscription is removed > > > > ~ > > > > The internal name of the CLT table has changed slightly, so the commit > > message needs updating. > > This change is done as part of 0002 review comment fixes patch. I will > let Dilip do this change when he merges the review comment fixes patch > to 0001 patch. > > > > > ====== > > > > src/backend/executor/execMain.c > > > > > > > > 11. > > > > + > > > > + /* > > > > + * Conflict log tables are managed by the system to record logical > > > > + * replication conflicts. We allow DELETE and TRUNCATE to permit > > > > users to > > > > + * manually prune these logs, but manual data insertion or modification > > > > + * (INSERT, UPDATE, MERGE) is prohibited to maintain the integrity of > > > > the > > > > + * system-generated logs. > > > > + * > > > > + * Since TRUNCATE is handled as a separate utility command, we only > > > > need > > > > + * to explicitly permit CMD_DELETE here. > > > > + */ > > > > + if (IsConflictNamespace(RelationGetNamespace(resultRel)) && > > > > + operation != CMD_DELETE) > > > > + ereport(ERROR, > > > > + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), > > > > + errmsg("cannot modify or insert data into conflict log table \"%s\"", > > > > + RelationGetRelationName(resultRel)), > > > > + errdetail("Conflict log tables are system-managed and only support > > > > cleanup via DELETE or TRUNCATE."))); > > > > > > > > It somehow feels backwards to check "operation != CMD_DELETE", with > > > > the obscure comment that TRUNCATE is handled elsewhere. > > > > > > > > How about just check if "(operation == CMD_INSERT || operation == > > > > CMD_UPDATE || operation == CMD_MERGE)". > > > > > > I felt the existing is ok here, as it is mentioned "we only need to > > > explicitly permit CMD_DELETE" . Are you seeing any commands other than > > > INSERT, UPDATE & MERGE possible here? > > > > 9. > > YMMV. > > > > No, I'm not seeing other commands. I guess the current code works. > > I preferred the current way in this case. > > > ====== > > src/backend/replication/logical/conflict.c > > > > > > 13c. > > > > TBH, I preferred code how it used to be -- where all the CLT constants > > > > and structs and enums and schemas were kept together. Now they are > > > > split across conflict.h and conflict.c making it harder to read as > > > > well as introducing need for static asserts that were not needed > > > > before. > > > > > > No change done, as this change is required. Amit has given the > > > explanation at [1]. > > > > > > > By refactoring the conflict functions into conflict.c, it means nearly > > everything is now kept together anyhow, just in the .c file instead of > > the .h file :-) > > No change done here because of the reason stated in the earlier mail. > > Rest of the comments were fixed. > The attached v37 version patch has the changes for the same. Also > Peter's comments on the documentation patch from [1] and Shveta's > comments from [2] are addressed in the attached patch. > > [1] - > https://www.postgresql.org/message-id/CAHut%2BPsrnU2BB1%2BM3c%2BDr5h62BLYfwBzhTg%3DBM7QtBoPwHYrKw%40mail.gmail.com > [2] - > https://www.postgresql.org/message-id/CAJpy0uCX53c40xopqmHtWSWBmh78BqhLVGXa88fU42eOi6w%2BLQ%40mail.gmail.com > Hi Vignesh,
I reviewed v37-0007 patch. Here is some review comments: 1. subinfo[i].subconflictlogdest is assigned multiple times: + if (PQgetisnull(res, i, i_sublogdestination)) + subinfo[i].subconflictlogdest = NULL; + else + subinfo[i].subconflictlogdest = + pg_strdup(PQgetvalue(res, i, i_sublogdestination)); + + if (PQgetisnull(res, i, i_sublogdestination)) + subinfo[i].subconflictlogdest = NULL; + else + subinfo[i].subconflictlogdest = + pg_strdup(PQgetvalue(res, i, i_sublogdestination)); 2. I think we should add a version check before: + appendPQExpBuffer(query, + "\n\nALTER SUBSCRIPTION %s SET (conflict_log_destination = %s);\n", + qsubname, + subinfo->subconflictlogdest); When we run pg_dump on a server with Postgres 18, we get the following output. ALTER SUBSCRIPTION sub2 SET (conflict_log_destination = (null)); Thanks, Shlok Kyal
