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

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.

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

~~~

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()?

~~~

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.

~~~

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.

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

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

~~~

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?

~~~

9.
+EXCEPTION WHEN insufficient_privilege THEN
+    RAISE NOTICE 'captured expected error: insufficient_privilege
during TRUNCATE';

Ditto previous review comment.

======
Kind Regards,
Peter Smith.
Fujitsu Australia