Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96) (envelope-from ) id 1wPE09-000Ytw-2F for pgsql-hackers@arkaria.postgresql.org; Tue, 19 May 2026 06:32:25 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.96) (envelope-from ) id 1wPE06-003xYV-1d for pgsql-hackers@arkaria.postgresql.org; Tue, 19 May 2026 06:32:23 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96) (envelope-from ) id 1wPE05-003xYM-2N for pgsql-hackers@lists.postgresql.org; Tue, 19 May 2026 06:32:23 +0000 Received: from mail-qt1-x82f.google.com ([2607:f8b0:4864:20::82f]) by makus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.98.2) (envelope-from ) id 1wPE02-00000000Hvq-46sU for pgsql-hackers@lists.postgresql.org; Tue, 19 May 2026 06:32:21 +0000 Received: by mail-qt1-x82f.google.com with SMTP id d75a77b69052e-5165195c8b0so39209551cf.0 for ; Mon, 18 May 2026 23:32:19 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1779172339; cv=none; d=google.com; s=arc-20240605; b=hIE7WnbjaTRXy4GCPhyDPu/XZq2uN8FLwtl05E+Qz33J6qKsa4OrynQBJtH+D89eKM OWDaYnShl7pt4QxnPK7csrunN3MEOLQuk/CwmtRix+4EOxfc6xovl6BdZVzOa0gDcYfO j3Z1RTzos7qlCsnnnZFCVRfvX1TbK/hrfPSkh1bYBQ9iKDx3Fjd3iqOp2AMPdwp4bS8Y HBCv2QIgLsZ9+D5n+FDxawrG7AWlx000kOzd7R3JebhtA1juPDQAPxRI7sbw5YahXu0y U665J363NprIsIT4M6ZJ6LmOnSx1XFg8nfinYdTwASxhE5gWrDctxa14xwfYSx6UeJ2E TEPQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20240605; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:dkim-signature; bh=KLDGKJ+N+TccVYm1Njh1QEXrXyUsjdca20zYNijLZko=; fh=FFlELFVQ02ZXrwS8KeemZRe7YQwPXIilFFhJYrK/Oz0=; b=kafKxbWdf7rTWfgwGbl8rrkoi4MFffu5nKmBCgc7LrMbYHaAl+KeVLK1i9A1N7unBk v6kjjFJybeX+cfnthlU3ysmraZi1Dp3ow6ZxcOI716Krh8OzsLNJxxFApG0V8zEBiH5f tnI9kVxEMHCqLR2tzkv/n+k8pVZim6pweD4WIumiaaZ4Ay4hr3InxHYKHM9WiBjrBT5O 8W7lBpkNnvbb9z0OvSfRGtA4jzrcgI5g+zRIq4iz46iXVKugZaseklnZRrzXyO5cX3lb 9M6NkwNIEZMpPiUYZk8dqo+uYngupI6Q8hZE06RDIuNnoMWE0C/tlgt51hEV2LiKI5TL f7AQ==; darn=lists.postgresql.org ARC-Authentication-Results: i=1; mx.google.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20251104; t=1779172339; x=1779777139; darn=lists.postgresql.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=KLDGKJ+N+TccVYm1Njh1QEXrXyUsjdca20zYNijLZko=; b=Je4ynRV2LKgIMnUQjwajxsqCvKvNTiw+WzhuOY1RgYCCWWBYv8AWSVLA7GozTkmcGo xsU+HLSAjllCIq0EImMTVclRDBSF0FlzjFHfCFR4mGVLM77AkNQqXAS6NY856zYpWa8U NQkgPQeuF9ZGNvXg/UD7uz3cAB1DVjxHzObc6RpJRfG/j78ziQn9V+y5ZORbEr+hf/UC QcfFL7fGvtOju6Lxkvy+e+CtOZ7GPy3viK/AYunviKwFwBR+A6cqlBl40DBhJg3Yhm0v NgWyhvQsKYA3q2uXRNRXtMjPeWfVtH9EA5wrff2S3qoc2U36Pm6HatluY6Hd+oa67V6h nimw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1779172339; x=1779777139; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-gg:x-gm-message-state:from :to:cc:subject:date:message-id:reply-to; bh=KLDGKJ+N+TccVYm1Njh1QEXrXyUsjdca20zYNijLZko=; b=koPNteIPV+/JuAXjvla2EhwoAiTGwRAC7+u0xXvi/pJ15XvipVxLLZ6ApwGR0kXF8X v8snBfD9m0NSZpSmClqEU+KHGRT9dJA9xS28uz6bGU3X/qN1aK1KrAVlgFJHZXGXi3z9 s7+/KE86Wdlap/TeEa7Dv7Kk92deDXiEvSMfvrZv/e/ZTaVLFvDb1ExQpAVSagD45PFB IyC3ZGYr28lGOVZWNblSliSKlpcSr6j62iH6uT0F0cYv2UGnSzzH7aJXH+C9PBNi3dba 20+F7DCnRBtMbzvH7qsmKNSSeO2vANOF5+h9J6OaMq0bFfXLxQdSdVfw6mFHKpQk7rfs HoEA== X-Forwarded-Encrypted: i=1; AFNElJ8pvRfDHvgX1f36G3MC7xOvyWq7D7+h0o3xvGmiQzIuRXy7uNWS6xs70421toYOO6hFBgsKbnvP7nv/SMAF@lists.postgresql.org X-Gm-Message-State: AOJu0YwTot/NE2SEl9kJOuclnfozeab2padKIMFyUcrlF5rXy2vRVz1B furSme33IMkfspXB4EhICjqQncfIVr4YjY6LhJJeFJoSwBwb37nbl7dIrww3KYDS76IdekcMkXS VdhFnMAwkUJX0dmA5v8rulsRbDfi+F0M= X-Gm-Gg: Acq92OF/25s5CCCj9skkCKKHagzyr16oR83yxsjU8hEcocZhTWmIOmWbthyGNzJYbVz uMmeLT579zCuF2Zne+giWtembB1ea0jjb04Pef4jxSM7h4F04HhnjuAZtubV1oKcp5MyLsnYB9l eXri0TdTLIBUQ89dqRe9eXFCO5c+p7e4uNYR0sEb1u23YhlsW10tU6Tl8g04pmgtBZjhr5x4mpU QOgjx8Q08cX3uaOaYQgZ5V/t9M2h4Vppo4ANuI6Jd4shvcG5chX1bMuIjLLOxJZj3C+l+x6nEwn HHXB+0I= X-Received: by 2002:a05:622a:50f:b0:50d:a6e3:ae1 with SMTP id d75a77b69052e-5165a03ee67mr253148541cf.17.1779172338655; Mon, 18 May 2026 23:32:18 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Peter Smith Date: Tue, 19 May 2026 16:31:51 +1000 X-Gm-Features: AVHnY4KxZDc-K7JpCl89-cVo7SRuo2_Qe4xa6iUsYpZohJU8n4r3zXvWE5CRc-Q Message-ID: Subject: Re: Proposal: Conflict log history table for Logical Replication To: vignesh C Cc: Dilip Kumar , Nisha Moond , Amit Kapila , shveta malik , Masahiko Sawada , Bharath Rupireddy , PostgreSQL Hackers , shveta malik Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk On Mon, May 18, 2026 at 10:35=E2=80=AFPM vignesh C wr= ote: > > On Wed, 13 May 2026 at 11:43, Peter Smith 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. =3D=3D=3D=3D=3D=3D 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_ is automatically created within a dedicated, system-managed 'pg_conflict' namespace to preve= nt 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 dropp= ed when the subscription is removed ~ The internal name of the CLT table has changed slightly, so the commit message needs updating. =3D=3D=3D=3D=3D=3D src/backend/catalog/heap.c 2. + * Don't allow creating relations in pg_catalog/pg_conflict directly, even + * though it is allowed to move user defined relations there. Semantics + * with search paths including pg_catalog are too confusing for now. I think "pg_catalog/pg_conflict" could be misinterpreted. Better to say "pg_catalog or pg_conflict". ~~~ 3. + if (!allow_system_table_mods && IsNormalProcessingMode()) + { + if ((IsCatalogNamespace(relnamespace) && relkind !=3D RELKIND_INDEX) || + IsToastNamespace(relnamespace)) + { + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("permission denied to create \"%s.%s\"", + get_namespace_name(relnamespace), relname), + errdetail("System catalog modifications are currently disallowed."))); + } + + if (IsConflictNamespace(relnamespace)) + { + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("permission denied to create \"%s.%s\"", + get_namespace_name(relnamespace), relname), + errdetail("Conflict schema modifications are currently disallowed."))); + } + } The curly-braces are unnecesary for those nested if-blocks. =3D=3D=3D=3D=3D=3D src/backend/catalog/namespace.c CheckSetNamespace: 4. + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("cannot move objects into or out of pg_conflict schema"))); Is it better to say "the pg_conflict schema". =3D=3D=3D=3D=3D=3D src/backend/commands/subscriptioncmds.c 5. - Looks like this was some unintended whitespace removal just after the static function forward declarations. ~~~ AlterSubscription: 6. + bool want_table =3D (opts.conflictlogdest =3D=3D CONFLICT_LOG_DEST_TABLE = || + opts.conflictlogdest =3D=3D CONFLICT_LOG_DEST_ALL); + bool has_oldtable =3D (old_dest =3D=3D CONFLICT_LOG_DEST_TABLE || + old_dest =3D=3D CONFLICT_LOG_DEST_ALL); These should be simplified using the new macro: CONFLICTS_LOGGED_TO_TABLE. =3D=3D=3D=3D=3D=3D src/backend/commands/tablecmds.c DropSubscription: 7. + ObjectAddress object; This can be declared at the lower scope closer to where it is actually used= . ~~~ 8. + if (OidIsValid(form->subconflictlogrelid)) + { + char *conflictrelname =3D get_rel_name(form->subconflictlogrelid); + /* There should be a blank line before that block comment. > > =3D=3D=3D=3D=3D=3D > > 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 user= s to > > + * manually prune these logs, but manual data insertion or modificatio= n > > + * (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 ne= ed > > + * to explicitly permit CMD_DELETE here. > > + */ > > + if (IsConflictNamespace(RelationGetNamespace(resultRel)) && > > + operation !=3D 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 !=3D CMD_DELETE", with > > the obscure comment that TRUNCATE is handled elsewhere. > > > > How about just check if "(operation =3D=3D CMD_INSERT || operation =3D= =3D > > CMD_UPDATE || operation =3D=3D 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. My previous review comment was because: 1. IMO, conditions that are positive instead of negative are easier to comprehend 2. It would make the checking code consistent with the comment =E2=80=9C(INSERT, UPDATE, MERGE) is prohibited=E2=80=9D, and with the error= message =E2=80=9Ccannot modify or insert=E2=80=9D. 3. Doing it the suggested way eliminates any need to mention that strange comment =E2=80=9CSince TRUNCATE=E2=80=A6=E2=80=9D CheckValidRowMarkRel: 10. + ereport(ERROR, + (errcode(ERRCODE_WRONG_OBJECT_TYPE), + errmsg("cannot lock rows in conflict log table \"%s\"", Should that say "in the"? =3D=3D=3D=3D=3D=3D 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 :-) ~~~ 11. +StaticAssertDecl(lengthof(ConflictLogSchema) =3D=3D NUM_CONFLICT_ATTRS, + "ConflictLogSchema length mismatch"); + + 11a. In fact, NUM_CONFLICT_ATTRS is not used outside this file, so now it can be defined right here. It means the assertion is unnecessary. Instead, the code here should look like: #define NUM_CONFLICT_ATTRS lengthof(ConflictLogSchema) ~ 11b. Unnecessary extra whitespace here. ~~~ create_conflict_log_table: 12. + Assert(relid !=3D InvalidOid); Favour using the macro OidIsValid(relid). =3D=3D=3D=3D=3D=3D src/include/catalog/pg_subscription.h 13. #include "catalog/objectaddress.h" #include "parser/parse_node.h" +#include "replication/conflict.h" I am guessing that this #include is probably no longer needed, because you removed the extern function that was using ConflictLogDest. =3D=3D=3D=3D=3D=3D src/include/replication/conflict.h 14. +/* Structure to hold metadata for one column of the conflict log table */ +typedef struct ConflictLogColumnDef +{ + const char *attname; /* Column name */ + Oid atttypid; /* Data type OID */ +} ConflictLogColumnDef; + AFAIK, you can move this into conflict.c now because it is only used in that file. ~~~ 15. +/* The single source of truth for the conflict log table schema */ +extern PGDLLIMPORT const ConflictLogColumnDef ConflictLogSchema[]; + AFAIK, you can remove this because all usages are now within conflict.c. ~~~ 16. +#define NUM_CONFLICT_ATTRS 11 + Move this into conflict.c -- see an earlier review comment. =3D=3D=3D=3D=3D=3D Kind Regards, Peter Smith. Fujitsu Australia