Re: Proposal: Conflict log history table for Logical Replication

Thanks for addressing all my previous review comment of v4.

Here are some more comments for the latest patch v5-0001.

======
GENERAL

1.
There are still a couple of place remainig where this new table was
not consistent called a "Conflict Log Table" (e.g. search for
"history")

e.g. Subject: [PATCH v5] Add configurable conflict log history table
for Logical Replication
e.g. + /* Insert conflict details to log history table. */
e.g. +-- CONFLICT LOG HISTORY TABLE TESTS

~~~

2.
Is automatically dropping the log tables always what the user might
want to happen? Maybe someone want them lying around afterwards for
later analysis -- I don't really know the answer; Just wondering if
this is (a) good to be tidy or (b) bad to remove user flexibility. Or
maybe the answer is leave if but make sure to add more documentation
to say "if you are going to want to do some post analysis then be sure
to copy this table data before it gets automatically dropped".

======
Commit message.

3.
User-Defined Table: The conflict log is stored in a user-managed table
rather than a system catalog.

~

I felt "User-defined" makes it sound like the user does CREATE TABLE
themselves and has some control over the schema. Maybe say
"User-Managed Table:" instead?

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

4.
#define SUBOPT_LSN 0x00010000
#define SUBOPT_ORIGIN 0x00020000
+#define SUBOPT_CONFLICT_LOG_TABLE 0x00040000

Whitespace alignment.

~~~

AlterSubscription:

5.
+ values[Anum_pg_subscription_subconflictlognspid - 1] =
+ ObjectIdGetDatum(nspid);
+ values[Anum_pg_subscription_subconflictlogtable - 1] =
+ CStringGetTextDatum(relname);
+
+ replaces[Anum_pg_subscription_subconflictlognspid - 1] = true;
+ replaces[Anum_pg_subscription_subconflictlogtable - 1] = true;

Something feels back-to-front, because if the same clt is being
re-used (like the NOTICE part taht follows) then why do you need to
reassign and say replaces[] = true here?

~~~

6.
+ /*
+ * If the subscription already has the conflict log table
+ * set to the exact same name and namespace currently being
+ * specified, and that table exists, just give notice and
+ * skip creation.
+ */

Is there a simpler way to say the same thing?

SUGGESTION
If the subscription already uses this conflict log table and it
exists, just issue a notice.

~~~

7.
+ ereport(NOTICE,
+ (errmsg("skipping table creation because \"%s.%s\" is already set as
conflict log table",
+ nspname, relname)));

I wasn't sure you need to say "skipping table creation because"... it
seems kind of internal details. How about just:

\"%s.%s\" is already in use as the conflict log table for this subscription

~~~

8.
+ /*
+ * Drop the existing conflict log table if we are
+ * setting a new table.
+ */

The comment didn't feel right by implying there is something to drop.

SUGGESTION
Create the conflict log table after dropping any pre-existing one.

~~~

drop_conflict_log_table:

9.
+ /* Drop the conflict log table if it exist. */

typo: /exist./exists./

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

10.
+static Datum
+tuple_table_slot_to_json_datum(TupleTableSlot *slot)
+{
+ HeapTuple tuple = ExecCopySlotHeapTuple(slot);
+ Datum datum = heap_copy_tuple_as_datum(tuple, slot->tts_tupleDescriptor);
+ Datum json;
+
+ if (TupIsNull(slot))
+ return 0;
+
+ json = DirectFunctionCall1(row_to_json, datum);
+ heap_freetuple(tuple);
+
+ return json;
+}

Bug? Shouldn't that TupIsNull(slot) check *precede* using that slot
for the tuple/datum assignments?

~~~

insert_conflict_log:

11.
+ Datum values[MAX_CONFLICT_ATTR_NUM];
+ bool nulls[MAX_CONFLICT_ATTR_NUM];
+ Oid nspid;
+ Oid relid;
+ Relation conflictrel = NULL;
+ int attno;
+ int options = HEAP_INSERT_NO_LOGICAL;
+ char *relname;
+ char *origin = NULL;
+ char *remote_origin = NULL;
+ HeapTuple tup;

I felt some of these var names can be confusing:

11A.
e.g. "conflictlogrel" (instead of 'conflictrel') would emphasise this
is the rel of the log file, not the rel that encountered a conflict.

~

11B.
Similarly, maybe 'relname' could be 'conflictlogtable', which is also
what it was called elsewhere.

~

11C.
AFAICT, the 'relid' is really the relid of the conflict log. So, maybe
name it as it 'confliglogreid', otherwise it seems confusing when
there is already parameter called 'rel' that is unrelated to thia
'relid'.

~~~

12.
+ if (searchslot != NULL)
+ values[attno++] = tuple_table_slot_to_json_datum(searchslot);
+ else
+ nulls[attno++] = true;
+
+ if (localslot != NULL)
+ values[attno++] = tuple_table_slot_to_json_datum(localslot);
+ else
+ nulls[attno++] = true;
+
+ if (remoteslot != NULL)
+ values[attno++] = tuple_table_slot_to_json_datum(remoteslot);
+ else
+ nulls[attno++] = true;

That function tuple_table_slot_to_json_datum() has potential to return
0. Is that something that needs checking, so you can assign nulls[] =
true?

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

13.
+char *
+get_subscription_conflict_log_table(Oid subid, Oid *nspid)
+{
+ HeapTuple tup;
+ Datum datum;
+ bool isnull;
+ char *relname = NULL;
+ Form_pg_subscription subform;
+
+ tup = SearchSysCache1(SUBSCRIPTIONOID, ObjectIdGetDatum(subid));
+
+ if (!HeapTupleIsValid(tup))
+ return NULL;
+
+ subform = (Form_pg_subscription) GETSTRUCT(tup);
+
+ /* Get conflict log table name. */
+ datum = SysCacheGetAttr(SUBSCRIPTIONOID,
+ tup,
+ Anum_pg_subscription_subconflictlogtable,
+ &isnull);
+ if (!isnull)
+ {
+ *nspid = subform->subconflictlognspid;
+ relname = pstrdup(TextDatumGetCString(datum));
+ }
+
+ ReleaseSysCache(tup);
+ return relname;
+}

You could consider assigning *nspid = InvalidOid when 'isnull' is
true, so then you don't have to rely on the caller pre-assigning a
default sane value. YMMV.

======
src/bin/psql/tab-complete.in.c

14.
- COMPLETE_WITH("binary", "connect", "copy_data", "create_slot",
+ COMPLETE_WITH("binary", "connect", "conflict_log_table",
"copy_data", "create_slot",

'conflict_log_table' comes before 'connect' alphabetically.

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

15.
+-- ok - change the conlfict log table name for existing subscription
already had old table
+ALTER SUBSCRIPTION regress_conflict_test2 SET (conflict_log_table =
'public.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';
+

typos in comment.
- /conlfict/conlflict/
- /for existing subscription already had old table/for an existing
subscription that already had one/

~~~

16.
+-- check new table should be created and old should be dropped

SUGGESTION
check the new table was created and the old table was dropped

~~~

17.
+-- ok (NOTICE) - try to set the conflict log table which is used by
same subscription
+ALTER SUBSCRIPTION regress_conflict_test2 SET (conflict_log_table =
'public.regress_conflict_log3');
+
+-- fail - try to use the conflict log table being used by some other
subscription
+ALTER SUBSCRIPTION regress_conflict_test2 SET (conflict_log_table =
'public.regress_conflict_log1');

Make those 2 comment more alike:

SUGGESTIONS
-- ok (NOTICE) - set conflict_log_table to one already used by this subscription
...
-- fail - set conflict_log_table to one already used by a different subscription

~~~

18.
Missing tests for describe \dRs+.

e.g. there are already dozens of \dRs+ examples where there is no clt
assigned, but I did not see any tests where the clt *is* assigned.

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