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

Hi Dilip.

Some review comments for v8-0001.

======
Commit message

1.
When the patches 0001 and 0002 got merged, I think the commit message
should have been updated also to say something along the lines of:

When ALL TABLES or ALL TABLES IN SCHEMA is used with publication won't
publish the clt.

======
src/backend/catalog/pg_publication.c

check_publication_add_relation:

2.
+ /* Can't be conflict log table */
+ if (IsConflictLogRelid(RelationGetRelid(targetrel)))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("cannot add relation \"%s\" to publication",
+ RelationGetRelationName(targetrel)),
+ errdetail("This operation is not supported for conflict log tables.")));

Should it also show the schema name of the clt in the message?

======
src/backend/commands/subscriptioncmds.c

3.
+/*
+ * Check if the specified relation is used as a conflict log table by any
+ * subscription.
+ */
+bool
+IsConflictLogRelid(Oid relid)

Most places refer to the clt. Wondering if this function ought to be
called 'IsConflictLogTable'.

======
src/backend/replication/logical/conflict.c

InsertConflictLogTuple:

4.
+ /* A valid tuple must be prepared and store into MyLogicalRepWorker. */

typo: /store into/stored in/

~~~

prepare_conflict_log_tuple:

5.
- index_close(indexDesc, NoLock);
+ oldctx = MemoryContextSwitchTo(ApplyContext);
+ tup = heap_form_tuple(RelationGetDescr(conflictlogrel), values, nulls);
+ MemoryContextSwitchTo(oldctx);

- return index_value;
+ /* Store conflict_log_tuple into the worker slot for inserting it later. */
+ MyLogicalRepWorker->conflict_log_tuple = tup;

5a.
I don't think you need the 'tup' variable. Just assign directly to
MyLogicalRepWorker->conflict_log_tuple.

~

5b.
"worker slot" -- I don't think this is a "slot".

======
src/backend/replication/logical/worker.c

6.
+ /* Open conflict log table. */
+ conflictlogrel = GetConflictLogTableRel();
+ InsertConflictLogTuple(conflictlogrel);
+ MyLogicalRepWorker->conflict_log_tuple = NULL;
+ table_close(conflictlogrel, AccessExclusiveLock);

Maybe that comment should say:
/* Open conflict log table and write the tuple. */


======
src/include/replication/conflict.h

7.
+ /* A conflict log tuple which is prepared but not yet inserted. */
+ HeapTuple conflict_log_tuple;
+

typo: /which/that/  (sorry, this one is my bad from a previous review comment)


======
src/test/regress/expected/subscription.out

8.
+-- ok - change the conflict log table name for an existing
subscription that already had one
+CREATE SCHEMA clt;
+ALTER SUBSCRIPTION regress_conflict_test2 SET (conflict_log_table =
'clt.regress_conflict_log3');
+SELECT subname, subconflictlogtable, subconflictlognspid = (SELECT
oid FROM pg_namespace WHERE nspname = 'public') AS is_public_schema
+FROM pg_subscription WHERE subname = 'regress_conflict_test2';
+        subname         |  subconflictlogtable  | is_public_schema
+------------------------+-----------------------+------------------
+ regress_conflict_test2 | regress_conflict_log3 | f
+(1 row)
+
+\dRs+
+

                    List of subscriptions
+          Name          |           Owner           | Enabled |
Publication | Binary | Streaming | Two-phase commit | Disable on error
| Origin | Password required | Run as owner? | Failover | Retain dead
tuples | Max retention duration | Retention active | Synchronous
commit |          Conninfo           |  Skip LSN  |  Conflict log
table

+------------------------+---------------------------+---------+-------------+--------+-----------+------------------+------------------+--------+-------------------+---------------+----------+--------------------+------------------------+------------------+--------------------+-----------------------------+------------+-----------------------
+ regress_conflict_test1 | regress_subscription_user | f       |
{testpub}   | f      | parallel  | d                | f
| any    | t                 | f             | f        | f
      |                      0 | f                | off
| dbname=regress_doesnotexist | 0/00000000 | regress_conflict_log1
+ regress_conflict_test2 | regress_subscription_user | f       |
{testpub}   | f      | parallel  | d                | f
| any    | t                 | f             | f        | f
      |                      0 | f                | off
| dbname=regress_doesnotexist | 0/00000000 | regress_conflict_log3
+(2 rows)

~

After going to the trouble of specifying the CLT on a different
schema, that information is lost by the \dRs+. How about also showing
the CLT schema name (at least when it is not "public") in the \dRs+
output.

~~~

9.
+-- ok - conflict_log_table should not be published with ALL TABLE
+CREATE PUBLICATION pub FOR TABLES IN SCHEMA clt;
+SELECT * FROM pg_publication_tables WHERE pubname = 'pub';
+ pubname | schemaname | tablename | attnames | rowfilter
+---------+------------+-----------+----------+-----------
+(0 rows)

Perhaps you should repeat this same test but using FOR ALL TABLES,
instead of only FOR TABLES IN SCHEMA

======
src/test/regress/sql/subscription.sql

10.
In one of the tests, you could call the function
pg_relation_is_publishable(clt) to verify that it returns false.

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