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 1vfIym-000o69-0E for pgsql-hackers@arkaria.postgresql.org; Mon, 12 Jan 2026 14:33:12 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.96) (envelope-from ) id 1vfIyk-000cBb-2y for pgsql-hackers@arkaria.postgresql.org; Mon, 12 Jan 2026 14:33:11 +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 1vfIyk-000cBS-1T for pgsql-hackers@lists.postgresql.org; Mon, 12 Jan 2026 14:33:10 +0000 Received: from mail-lf1-x12a.google.com ([2a00:1450:4864:20::12a]) by makus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.96) (envelope-from ) id 1vfIyh-0002C1-2C for pgsql-hackers@lists.postgresql.org; Mon, 12 Jan 2026 14:33:09 +0000 Received: by mail-lf1-x12a.google.com with SMTP id 2adb3069b0e04-59b7882e605so5616142e87.0 for ; Mon, 12 Jan 2026 06:33:08 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1768228387; x=1768833187; 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=U2C270v9OtyCYtgEoIgmfAdV654VV290zl50KMg16DI=; b=apMKsH3+r/P+IDL4DCnKu8/YPjpJIM4GgyM8CjNVV9mKtyoH0bDbuSkMxoleL0MCC+ VAm7DtG7N8YLS5u/fOLCXR9wFJ2L7LSFqrsYcgHrvqwIfU1/S5QdmbD8XaOf2P65jMXT wikO/AFjB/5HRBRu6WSzXmtVn09Fj3pUked8RiQltesO4TiuQfP36QP2sTbtaI+/1U1C JA5ov+uzcQwZtpwXC520zfrJEByFaJeWmUl3bdpv5Wq0dAZWFAesk823GM5gSCeaT7iD H2iXPwztReuFEIE9pmIEd9KGY2aMgnhqTPz70AiemS5u9ge0sAaKKbE39TN5N+b5XbTW gKQQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1768228387; x=1768833187; 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=U2C270v9OtyCYtgEoIgmfAdV654VV290zl50KMg16DI=; b=HlrL3l8hn9Alp3Y556meL0Zr7xadoun00fS1ognwK8AqMnDsjCh6Kkh/HL5r0wnRG8 EW12Jzw/bLpnxaQ78zjcRg9eJ9t+ta2e3hP97ePFUI7PnzUHU9JRxWgejWannqqOHePt eX+nPrVelzA9eEyLg+PCnCr6e1zffDB74kxzMeKTbowm/8Kq68JgfX8Tby6N7GP3DrhM EnVzExUzXL25IAOmQ7TN06uz7XCILFwFYwHJXfp92HRvuN3iiRkWqyJiI4gX/DuEjlEy RaKSSjFGSIYRucxLZayJ8PKUZMUef/7rm6CW92ngOooz4e9DfSnBpYcjWG24G+73yZPr CtKg== X-Forwarded-Encrypted: i=1; AJvYcCUIKaoD2Q25xy/5UWNfBaMo3CfyJQifqmBhtF/bXFWz9kvHg+dtrv8uO4m3IBh62HsplFfQUHo/d4SrplC2@lists.postgresql.org X-Gm-Message-State: AOJu0Yzws3wOLP6mEuY4/A10cXVp155E9JW/WLLqBoOqqfA4oYJAUaGX XZstCRlBIVUdz5iyrG2LErpB+nAp8cuiqBM2WyS/Y6XpsaxFcsRrXv4E26KMj6ooCvDWgZENpv7 jpPVNFLEMX172MB3WpadyL/HUkXqcYW4= X-Gm-Gg: AY/fxX5wdKEJgvKbv6+aAD8hgo/4K8LqPHzv0ps25XzuAVaeF5icb1VX1a0+88b+EEu v5oe4YFtK1nUj0PzjjFGr3VugEAAn38hGBQLfpciA79haeIbpa7z9Q6w+kzJ7bkoEluDz4Po/oG t/UR+eYoZAPW/zq4qsToJKmQA5ckLi6kfEoDx5R6afVpH29FhXzYtB6jv5U9HTOXwyKw+fIyLpT Q9LHvBLMvKx7GcBK5iaK6YwnOCi2LjBk4Qca/l3s019wNOyo4adoKQfH6Mcs2ICM3HuT7GtT8MH wkoHaLIEVQQJsVJEXuCz3PlCeuw4GnamdwyHUMw= X-Google-Smtp-Source: AGHT+IHQiKlsicoBJ3uC1+5Kta38XztsRP74x6Y/LKphd7LOPBiR8dfOg1xtNQJuNFUcudBm+rsXGkLHyDfmf3V8C2o= X-Received: by 2002:a2e:be13:0:b0:37b:9a1d:dee3 with SMTP id 38308e7fff4ca-382ff6a52aamr52014101fa.16.1768228386337; Mon, 12 Jan 2026 06:33:06 -0800 (PST) MIME-Version: 1.0 References: In-Reply-To: From: Dilip Kumar Date: Mon, 12 Jan 2026 20:02:49 +0530 X-Gm-Features: AZwV_QgN4JCa3bN_p8xXbBN_lZEv41OxARlde7gKtbgveYm9hZVqdMn1oGj2TNg Message-ID: Subject: Re: Proposal: Conflict log history table for Logical Replication To: Peter Smith Cc: vignesh C , shveta malik , Amit Kapila , Masahiko Sawada , Bharath Rupireddy , PostgreSQL Hackers 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, Jan 12, 2026 at 10:56=E2=80=AFAM Peter Smith wrote: > > Some review comments for the v20-0001 patch. > > =3D=3D=3D=3D=3D=3D > 0. General > > Applying the patch gives whitespace warnings: > > $ git apply ../patches_misc/v20-0001-Add-configurable-conflict-log-table-= for-Logical-.patch > ../patches_misc/v20-0001-Add-configurable-conflict-log-table-for-Logical-= .patch:1543: > trailing whitespace. > CREATE SUBSCRIPTION regress_conflict_protection_test CONNECTION > 'dbname=3Dregress_doesnotexist' > ../patches_misc/v20-0001-Add-configurable-conflict-log-table-for-Logical-= .patch:610: > new blank line at EOF. > + > warning: 2 lines add whitespace errors. Fixed > =3D=3D=3D=3D=3D=3D > src/backend/catalog/pg_publication.c > > 1. > + > + /* Can't be conflict log table */ > + if (IsConflictNamespace(RelationGetNamespace(targetrel))) > + ereport(ERROR, > + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), > + errmsg("cannot add relation \"%s.%s\" to publication", > + get_namespace_name(RelationGetNamespace(targetrel)), > + RelationGetRelationName(targetrel)), > + errdetail("This operation is not supported for conflict log tables.")))= ; > > I felt it may be better to still keep the function > IsConflictLogTable(). You can just put this namespace logic inside > that function. Then one day, if other tables are added to that special > schema, at least the impact for checking CLT is contained. I prefer not to keep that additional function, anyway all the checks are done using isConflictClass and friends, and in future if we need to change where something else can come in this conflict namespace it wouldn't be hard to change the logic. > ~~~ > > is_publishable_relation: > > 2. > /* > * Another variant of is_publishable_class(), taking a Relation. > + * > + * Note: Conflict log tables are not publishable. However, we intention= ally > + * skip this check here because this function is called for every change= and > + * performing this check during every change publication is costly. To = ensure > + * unpublishable entries are ignored without incurring performance overh= ead, > + * tuples inserted into the conflict log table uses the HEAP_INSERT_NO_L= OGICAL > + * flag. This allows the decoding layer to bypass these entries automat= ically. > > Is that comment still relevant, now that is_publishable_class() has > been changed to use IsConflictClass()? Yeah this is not relevant anymore. > ~~~ > > pg_relation_is_publishable: > > 3. > + > + /* Subscription conflict log tables are not published */ > result =3D is_publishable_class(relid, (Form_pg_class) GETSTRUCT(tuple)= ); > > The comment seems unnecessary/misplaced because there is no special > code anymore in this function for CLT. Right > ~~~ > > GetAllPublicationRelations: > > 4. > + /* Subscription conflict log tables are not published */ > if (is_publishable_class(relid, relForm) && > > The comment seems unnecessary/misplaced because there is no special > code anymore in this function for CLT. Fixed > =3D=3D=3D=3D=3D=3D > src/include/replication/conflict.h > > 5. > +extern Relation GetConflictLogTableInfo(ConflictLogDest *log_dest); > +extern void InsertConflictLogTuple(Relation conflictlogrel); > > This change seems to be in the wrong patch. afaict these functions are > not implemented in patch 0001. Fixed > =3D=3D=3D=3D=3D=3D > src/test/regress/sql/subscription.sql > > 6. > +-- verify the physical table exists, its OID matches subconflictlogrelid= , > +-- and it is located in the 'pg_conflict' namespace. > +SELECT count(*) > +FROM pg_class c > +JOIN pg_subscription s ON c.relname =3D 'pg_conflict_' || s.oid > +JOIN pg_namespace n ON c.relnamespace =3D n.oid > +WHERE s.subname =3D 'regress_conflict_test1' > + AND c.oid =3D s.subconflictlogrelid > + AND n.nspname =3D 'pg_conflict'; > + count > +------- > + 1 > +(1 row) > + > > For this kind of test, I wondered if it would be better to make the > SQL read more like the comment says. You can put some of the WHERE > conditions directly in the SELECT column list and just let the > regression comparisons with 'expected' results take care of validating > them. > > e.g. > SUGGESTION > -- verify the physical table exists, its OID matches subconflictlogrelid, > -- and it is located in the 'pg_conflict' namespace. > SELECT n.nspname, (c.oid =3D s.subconflictlogrelid) AS "Oid matches" > FROM pg_class c > JOIN pg_subscription s ON c.relname =3D 'pg_conflict_' || s.oid > JOIN pg_namespace n ON c.relnamespace =3D n.oid > WHERE s.subname =3D 'regress_conflict_test1'; > nspname | Oid matches > -------------+------------- > pg_conflict | t > (1 row) > > ~~~ > > 7. > Similarly, for the CLT metadata test, instead of only expecting 11 > rows, why not just SELECT/compare all the attributes more generically? > > e.g. > SUGGESTION > -- check if the internal table has the correct schema > SELECT a.attnum, a.attname > FROM pg_attribute a > JOIN pg_class c ON a.attrelid =3D c.oid > JOIN pg_subscription s ON c.relname =3D 'pg_conflict_' || s.oid > WHERE s.subname =3D 'regress_conflict_test1' AND a.attnum > 0 > ORDER BY a.attnum; > attnum | attname > --------+------------------- > 1 | relid > 2 | schemaname > 3 | relname > 4 | conflict_type > 5 | remote_xid > 6 | remote_commit_lsn > 7 | remote_commit_ts > 8 | remote_origin > 9 | replica_identity > 10 | remote_tuple > 11 | local_conflicts > (11 rows) Make sense > ~~~ > > 8. > +EXCEPTION WHEN insufficient_privilege THEN > + RAISE NOTICE 'captured expected error: insufficient_privilege > during ALTER'; > > IIUC, the test was only written that way because the CLT name is > dynamically based on the , so you cannot know it up-front to > include that name in the 'expected' error message. Maybe there could > be a comment to explain that? Right that's the reason, added comments for the same > ~~~ > > 9. > +EXCEPTION WHEN insufficient_privilege THEN > + RAISE NOTICE 'captured expected error: insufficient_privilege > during TRUNCATE'; > > Ditto previous review comment. Done Will send updated patch after checking comments in other emails. --=20 Regards, Dilip Kumar Google