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 1wPeVm-000vUG-2c for pgsql-hackers@arkaria.postgresql.org; Wed, 20 May 2026 10:50:51 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.96) (envelope-from ) id 1wPeVk-007BS5-1g for pgsql-hackers@arkaria.postgresql.org; Wed, 20 May 2026 10:50:49 +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 1wPeVj-007BRs-3B for pgsql-hackers@lists.postgresql.org; Wed, 20 May 2026 10:50:49 +0000 Received: from mail-oi1-x236.google.com ([2607:f8b0:4864:20::236]) by makus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.98.2) (envelope-from ) id 1wPeVi-00000000TTN-0Z3T for pgsql-hackers@lists.postgresql.org; Wed, 20 May 2026 10:50:48 +0000 Received: by mail-oi1-x236.google.com with SMTP id 5614622812f47-479d593a0c3so3985216b6e.0 for ; Wed, 20 May 2026 03:50:46 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1779274246; cv=none; d=google.com; s=arc-20240605; b=aV5kUnC1cbvoEqtvYmUNmszODPbwNLbXLR6Mmq89UCyl4pjQ5KLUF4aV2qtvcovnTh 1Fd7W9LHjFJy4jCtNop5SFT+XuskbNau1hXwt5vDkW6rhtpFErBiz4LV2qYPis0tlOxB AZvbHN/2dkLTbt4H4LwR3MrqYfgMwaqlXIN/gnbcmqYZJWe0YWi+NJfe3tI/tKlVjx5w gettrP4yqO7lDduSJyslBGW+rvlObsf59g8CtVYQfOoI55gyoQICOgFZJ6L5Ov04Q/C4 hkQkK41S9a8Juoe1o8jk1e4b+3kbXVm3IMO5n8qM1QWdcuPK8sPPVuSvoRGan7PSsZMi bEOA== 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=+4q6ujhN35EUWHYkDjxhtBk66ZxbBYydO/TbtmSC7TA=; fh=4klBboRL0pGSJDH6y0M11piKQmLwtgNbwhkM+oauUrI=; b=cOo6Xss1E1UmWoT9EeLpAnomZLxFkMU8jFSDfA2TwHSgwMvBh5kyk3PzRZyjbU5VIy JutM3k4IC3QfCQSJ//ZV0x3BNu5GU/EbBgq52Hrg67I8yGBV1zvuDTriQtZWdULmzZ6Q jpervQvvABvPm9lvY8+nPFVI5oc63gjeXbtlQzDdSjgVda0by6Qqs/FGh87auFDIagFn OeFVi++9F9V0bvLUykXbYJYf1h0bW2334I3cnYfrvRgRs1Ux62y0BfSqeTRb0uVRCo07 H6USN3f70WNxtHMOdnes5Rim05b9XdnXDiU/bf6s1pR/qARmTn7+NA+Q/n4MRheoZSdM nGbg==; 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=1779274246; x=1779879046; 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=+4q6ujhN35EUWHYkDjxhtBk66ZxbBYydO/TbtmSC7TA=; b=ainQkrl88MoylII4/iH4q6m6MfmcX+37QiQ9L/gTxPLzT0hUlVaKJCA5LdEvXHlyqI X+AbzO9waLuhoHIY0TlzGMRcyr0dmVvja2qsuPS7QlYEVa3/GsIX/G+n6SbtideYBbGj C0+zfZ5a/XF6XoMjXoTEOfqr786EgJtVxED9Zb/jFSx8ckdhUCxR3juUaxH9Q7GC4kJo P+VMzfdEAiMK/+suMKNJ/dC0nGApPwKfzAzxjCfdYeYNK/g6hz8/XKhFcn/mPC8W1A7x 2QouUnrz8p6nRM3hj9RzvuWaTML+povc8LvOaQW2a/moGi2IARLn1F43rAA67io+jbs7 6qNw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1779274246; x=1779879046; 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=+4q6ujhN35EUWHYkDjxhtBk66ZxbBYydO/TbtmSC7TA=; b=Dg9WZC6fxKeyBFCI9J2P7GSHaaxKzCK72L+/wApJJTTpud7jWtPDt3I6dBXi39dO7F PQjFP6Gizy80uH9LlmOb9SPlK1t4E55Y2FJcIlhzWFZG7J34iM4+E5Ijz4pNJhIZoTXj ddxSgVHdjLGBoTLXd+u936KSBbHQkJTKW+kZqsOfpog5X32XI630BlStMIeA+YwG8Tzg TxVcg3EcptPouEtWTL0x3QAqA4pyRYzYO5qHl+WsjQA/n9lrpZTXNU871bMNLbfm+g1Y DF58Foe8kSUNk2G4u6oXvgPEdgl2P6wWLPHkCUOlQiKoOkC9tiTgx/h8zosztZNinxEc tz7w== X-Forwarded-Encrypted: i=1; AFNElJ+DDvJzYViQSUMhBxRqwLU1+VMgLsvAz6zDXRDcKdxdMHcspYIDaZ25DUR1iy9see6UebbMltAJrxeaNBDV@lists.postgresql.org X-Gm-Message-State: AOJu0YzebCAJSqTx79Q1qdHVjco7QaH6T4l0BE/Pz8qcayG8xy8kDMST t6yAdEsndjNZsjDHFvgE6ZOJGA7m2QdZawcdVFShD8iUYBiLC3irMbPGWhvYciG3aJwxDsLVsUn bWSou5fsmQHEE9OuCCXzlw5FjWPiLs5U= X-Gm-Gg: Acq92OGplI0rHc2RB4Nq1JP0xpTR6CEnTuN0UEYMPEwytMejVg6aTH1FnXXcSVaoxtc ibTW454hTtXlv+OVmK9aDTIsGe4IPL38xhhnVCP7R1ZD9ixv+IsBuj/+atO0gyHptxVcbkzBaT3 XKWxTt78rkhUjitdLGxbe9WKy7Vb9L26o2PKf7lbPo10bBJJtTgpMjrHpQJgtZzzYls7Cb85fFn nRXtrynTVWOiYLS/erHss89F+DBzh84ylWzhFMkOYAM5AF6CuU4tbrYBiqliG7ZImBZ+g4v2jTr 8UYFxw4DPA== X-Received: by 2002:a05:6808:158f:b0:467:153a:2d9c with SMTP id 5614622812f47-482cb7bfabbmr15675776b6e.15.1779274246296; Wed, 20 May 2026 03:50:46 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Shlok Kyal Date: Wed, 20 May 2026 16:20:34 +0530 X-Gm-Features: AVHnY4Lqnp4wbAFMyNr-EFnLFqdjPzaSp2w9a9Oh7pAeAEIuNKJhle1lAJU7CDM Message-ID: Subject: Re: Proposal: Conflict log history table for Logical Replication To: vignesh C Cc: Peter Smith , 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 Wed, 20 May 2026 at 15:05, vignesh C wrote: > > On Tue, 19 May 2026 at 12:02, Peter Smith wrote: > > > > On Mon, May 18, 2026 at 10:35=E2=80=AFPM vignesh C wrote: > > > > > > On Wed, 13 May 2026 at 11:43, Peter Smith wro= te: > > > > 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 patch= es. > > > > =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 automaticall= y > > created within a dedicated, system-managed 'pg_conflict' namespace to p= revent > > users from manually dropping or altering it. This also prevents acciden= tal > > name collisions with user-created tables. This table is linked to the > > subscription via an internal dependency, ensuring it is automatically d= ropped > > 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. > > > > > =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 = users to > > > > + * manually prune these logs, but manual data insertion or modific= ation > > > > + * (INSERT, UPDATE, MERGE) is prohibited to maintain the integrity= of the > > > > + * system-generated logs. > > > > + * > > > > + * Since TRUNCATE is handled as a separate utility command, we onl= y need > > > > + * 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 suppor= t > > > > cleanup via DELETE or TRUNCATE."))); > > > > > > > > It somehow feels backwards to check "operation !=3D CMD_DELETE", wi= th > > > > 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 tha= n > > > 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. > > > =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 consta= nts > > > > 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%2BDr5= h62BLYfwBzhTg%3DBM7QtBoPwHYrKw%40mail.gmail.com > [2] - https://www.postgresql.org/message-id/CAJpy0uCX53c40xopqmHtWSWBmh78= BqhLVGXa88fU42eOi6w%2BLQ%40mail.gmail.com > Hi Vignesh, Here are some minor comments: Comment for all patches. 1. At multiple places (code comments and test cases) we are using the word 'internal conflict log table'. Do we need to use the word 'internal'? I think using 'conflict log table' is sufficient? Comments for 0002: 2. We can rename the schema pg_conflict to a different schema name. Is it ok to hardcode the schema name to 'pg_conflict'? - errmsg("cannot move objects into or out of CONFLICT schema= "))); + errmsg("cannot move objects into or out of pg_conflict schema"))); Example: postgres=3D# ALTER SCHEMA pg_conflict RENAME TO sc1; ALTER SCHEMA postgres=3D# ALTER TABLE t2 SET SCHEMA sc1; ERROR: cannot move objects into or out of pg_conflict schema Comment for 0005/0006: 3. static const char *const ConflictTypeNames[] =3D { [CT_INSERT_EXISTS] =3D "insert_exists", [CT_UPDATE_ORIGIN_DIFFERS] =3D "update_origin_differs", [CT_UPDATE_EXISTS] =3D "update_exists", [CT_UPDATE_MISSING] =3D "update_missing", [CT_DELETE_ORIGIN_DIFFERS] =3D "delete_origin_differs", [CT_UPDATE_DELETED] =3D "update_deleted", [CT_DELETE_MISSING] =3D "delete_missing", [CT_MULTIPLE_UNIQUE_CONFLICTS] =3D "multiple_unique_conflicts" }; There are a few extra blank lines after declaration of ConflictTypeNames. Thanks, Shlok Kyal