Re: Proposal: Conflict log history table for Logical Replication - Mailing list pgsql-hackers

On Mon, Jan 12, 2026 at 10:56 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Some review comments for the v20-0001 patch.
>
> ======
> 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=regress_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

> ======
> 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 intentionally
> + * 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 overhead,
> + * tuples inserted into the conflict log table uses the HEAP_INSERT_NO_LOGICAL
> + * flag.  This allows the decoding layer to bypass these entries automatically.
>
> 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 = 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

> ======
> 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

> ======
> 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 = 'pg_conflict_' || s.oid
> +JOIN pg_namespace n ON c.relnamespace = n.oid
> +WHERE s.subname = 'regress_conflict_test1'
> +  AND c.oid = s.subconflictlogrelid
> +  AND n.nspname = '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 = s.subconflictlogrelid) AS "Oid matches"
> FROM pg_class c
> JOIN pg_subscription s ON c.relname = 'pg_conflict_' || s.oid
> JOIN pg_namespace n ON c.relnamespace = n.oid
> WHERE s.subname = '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 = c.oid
> JOIN pg_subscription s ON c.relname = 'pg_conflict_' || s.oid
> WHERE s.subname = '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 <subid>, 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.

--
Regards,
Dilip Kumar
Google