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 1wPxb3-001B0x-2x for pgsql-hackers@arkaria.postgresql.org; Thu, 21 May 2026 07:13:34 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.96) (envelope-from ) id 1wPxb1-009WXL-2J for pgsql-hackers@arkaria.postgresql.org; Thu, 21 May 2026 07:13:32 +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 1wPxb1-009WXD-0b for pgsql-hackers@lists.postgresql.org; Thu, 21 May 2026 07:13:32 +0000 Received: from mail-oa1-x34.google.com ([2001:4860:4864:20::34]) by makus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.98.2) (envelope-from ) id 1wPxaz-000000003zi-2GYd for pgsql-hackers@lists.postgresql.org; Thu, 21 May 2026 07:13:31 +0000 Received: by mail-oa1-x34.google.com with SMTP id 586e51a60fabf-439cae0dc81so1502089fac.1 for ; Thu, 21 May 2026 00:13:29 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1779347608; cv=none; d=google.com; s=arc-20240605; b=gWR7wTYCE9hw7NHW2ra/lJ/vjBlwY2tM3W8BJQ6kBjmBk7u6uszVTHUEb24uoWde1u ORa+cY82BKSdDVXe4hECA1JvOe/Rc98xGM39gOw2Ff96sCvpavagt3tgHxNMJwAhLbf4 F+gVdzxsKqNHiOTJ5ZYm7SZpq8IXOWyGKTqdH7uZu4YzctOz1nn6cAvt3vWj7Y+4iihI xUS+A2fg823bWOKkSHSv4ZchMOzyHPfCnH75dsFbJ9tGZuNvgxozry7RvbUrEYHdfHf1 v19Bh2/cRloVzjRHGZAEIoBf671GwRpXt2bF9sxt6gplc2iW/RtVNDb5yKsF1A2E3Osk ilEw== 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=UiQm/ywhgeS+kCK+pinzRPyc9NhykC4sejyw1gs3s6A=; fh=NMMyZVR9J2JqVbndEQSH3TicYUkrhovSuI/05F7DaGY=; b=eH3XLBRdDCx5A8OIFvG0QG+j72ft/iOf1BUdt+navZ0AjoXTefLx1I0lsdfdUFMW9j AJCyGADGmH3qed4db8gL+VTRjB/lrLNGvgHRXyFV9eiGQ9csJ10iy5x2vXKScsZLWHop REjb+D5yByeB0csDH0V6zoyXbOjLYbBOyd/XW2+hHLWYAqk/qELScMoK4EOZHildVOZD QJNEbbcDmigQn+5MAaGuA/Q7NHklml/Dn6jJQt0AgCQgBNTcEYdeIppdVe6zenrzGUPA TY7UQJkIQ6mMhRbapivdCylne8h3pbCcvjtgSbeSUmIvZIMYr1GhxC1+CpuymXeBzM7X duEQ==; 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=1779347608; x=1779952408; 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=UiQm/ywhgeS+kCK+pinzRPyc9NhykC4sejyw1gs3s6A=; b=qUuZltg7kgwmG20MpIBJGB4LZUh+yoxNHCah8iJaDJvKypKaD8Nt24Wv3AiNxvwmp3 awE+n2EIKNo/FBJ081UA5SDUstlkVctXyaOjAe6n7ew81h/Ed8cabe4BFUwCdxeoOSKb MzX2FH9Cn6iKBqb/ij4OZSeVefqsBt6th1DEauMPiz3oQkDzGqy9QPdddvMsLhenme4W qAzHIpkfSIoQNrCezOZ2DLOmAZDYwlpPUm+vO96gnlyAjfEVEoCQbozY7gE9bW7/ummy TZmEALLCzaknoQKOTtJC09E5S99VdaId2DSj7hzmy/DuYwov/jxD/R9U3dfXModujcOF lTtA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1779347608; x=1779952408; 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=UiQm/ywhgeS+kCK+pinzRPyc9NhykC4sejyw1gs3s6A=; b=Oxcaukd+Ny+NpUkVSQHTeJ6rXWB4x27EEB4ikV/mKolFIM272d/x4oEF8k1fb1gUa1 e4lTDE6cHrK5VBVFabpIbInHU+NIUXpnHeLyDoDaZofuvzho4i9AJa0C9uPfXDjV03QZ heY9SVMlTtuf0aWTGHZYwQfKJ138xGWDYubY5Gb8fdkogs7Me4cFPX5/KxOlYa8g7QXe /EOP7bx54uihfa91TVpFMbD9Pkq1OZgSZA53fNkGcfoAk1ejAfyUJ7z1bcIUoJz+m1Fh PwJa14ptA/Cg5hAballWKwbKPht4Wxy7OPAEAYwU5BvMV2u31FwVDJqLCrNyNlvBYZ9m av0A== X-Forwarded-Encrypted: i=1; AFNElJ99csO/XnlAL3gQbp66u1D/cOJ+X9L+RSAI5qf/+jQ0EJV2/V0usB0j1ry3jXC5pwg6+pa2z5OJfBPwCaD0@lists.postgresql.org X-Gm-Message-State: AOJu0YxhYp/LeqgEdIpdoX8URVQbNrIV2YqzSHFPRc9H7G/+nfVisE8J dm9THXMm07SOj1VqeNkusjv6cTHQ1sBfI1wjdK1ER9urDNSCdrGyjv5kSSxI3PU3SSY3Ac+xSok aVMoNo2VxouDqLSn/MHbJsCtj24cWyow= X-Gm-Gg: Acq92OFdU2uDrCdGBx3qD5d8JDLgr69YMivAHtD1eaWQtQSl/WaC24gxuVxz11F5UEy xhtHv91bVQ7InG6CAuQx9BHjYGKR97zi+nzhuyc8AzGGKlCEsBVZgvyYVY6fr744KorBSn/I543 L1q0U1BhrRXpPfYmZDHBESWK7azHN9te5yvNWh3/+D98Qms5cVCPGemSpJJRPTM2Fhm9I3tlE+X VwL3CsVx0pKVjSUw8/+FaekXmmttE245XyneiG0pj0AX3drwnhhef7th57iU13c1kJzk+POaCyX lLv3q19a X-Received: by 2002:a05:6870:6b97:b0:42f:fc34:78bd with SMTP id 586e51a60fabf-43b2eb7474fmr1165628fac.33.1779347608387; Thu, 21 May 2026 00:13:28 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Shlok Kyal Date: Thu, 21 May 2026 12:43:15 +0530 X-Gm-Features: AVHnY4JD67uhfzOro-8eHY3OpxBKJ0zdW1PYwqiRIh9KYunnn5a3JLKahQ4aJH0 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, 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 =3D NULL; + else + subinfo[i].subconflictlogdest =3D + pg_strdup(PQgetvalue(res, i, i_sublogdestination)); + + if (PQgetisnull(res, i, i_sublogdestination)) + subinfo[i].subconflictlogdest =3D NULL; + else + subinfo[i].subconflictlogdest =3D + 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 =3D %s);\n", + qsubname, + subinfo->subconflictlogdest); When we run pg_dump on a server with Postgres 18, we get the following outp= ut. ALTER SUBSCRIPTION sub2 SET (conflict_log_destination =3D (null)); Thanks, Shlok Kyal