Re: Proposal: Conflict log history table for Logical Replication

Hi Dilip.

I started to look at this thread. Here are some comments for patch v4-0001.

=====
GENERAL

1.
There's some inconsistency in how this new table is called at different times :
a) "conflict table"
b) "conflict log table"
c) "conflict log history table"
d) "conflict history"

My preference was (b). Making this consistent will have impacts on
many macros, variables, comments, function names, etc.

~~~

2.
What about enhancements to description \dRs+ so the subscription
conflict log table is displayed?

~~~

3.
What about enhancements to the tab-complete code?

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

4.
#define SUBOPT_MAX_RETENTION_DURATION 0x00008000
#define SUBOPT_LSN 0x00010000
#define SUBOPT_ORIGIN 0x00020000
+#define SUBOPT_CONFLICT_TABLE 0x00030000

Bug? Shouldn't that be 0x00040000.

~~~

5.
+ char *conflicttable;
XLogRecPtr lsn;
} SubOpts;

IMO 'conflicttable' looks too much like 'conflictable', which may
cause some confusion on first reading.

~~~

6.
+static void CreateConflictLogTable(Oid namespaceId, char *conflictrel);
+static void DropConflictLogTable(Oid namespaceId, char *conflictrel);

AFAIK it is more conventional for the static functions to be
snake_case and the extern functions to use CamelCase. So these would
be:
- create_conflict_log_table
- drop_conflict_log_table

~~~

CreateSubscription:

7.
+ /* If conflict log table name is given than create the table. */
+ if (opts.conflicttable)
+ CreateConflictLogTable(conflict_table_nspid, conflict_table);
+

typo: /If conflict/If a conflict/

typo: "than"

~~~

AlterSubscription:

8.
- SUBOPT_ORIGIN);
+ SUBOPT_ORIGIN |
+ SUBOPT_CONFLICT_TABLE);

The line wrapping doesn't seem necessary.

~~~

9.
+ replaces[Anum_pg_subscription_subconflictnspid - 1] = true;
+ replaces[Anum_pg_subscription_subconflicttable - 1] = true;
+
+ CreateConflictLogTable(nspid, relname);
+ }
+

What are the rules regarding replacing one log table with a different
log table for the same subscription? I didn't see anything about this
scenario, nor any test cases.

~~~

CreateConflictLogTable:

10.
+ /*
+ * Check if table with same name already present, if so report an error
+ * as currently we do not support user created table as conflict log
+ * table.
+ */

Is the comment about "user-created table" strictly correct? e.g. Won't
you encounter the same problem if there are 2 subscriptions trying to
set the same-named conflict log table?

SUGGESTION
Report an error if the specified conflict log table already exists.

~~~

DropConflictLogTable:

11.
+ /*
+ * Drop conflict log table if exist, use if exists ensures the command
+ * won't error if the table is already gone.
+ */

The reason for EXISTS was already mentioned in the function comment.

SUGGESTION
Drop the conflict log table if it exists.

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

12.
+static Datum TupleTableSlotToJsonDatum(TupleTableSlot *slot);
+
+static void InsertConflictLog(Relation rel,
+ TransactionId local_xid,
+ TimestampTz local_ts,
+ ConflictType conflict_type,
+ RepOriginId origin_id,
+ TupleTableSlot *searchslot,
+ TupleTableSlot *localslot,
+ TupleTableSlot *remoteslot);

Same as earlier comment #6 -- isn't it conventional to use snake_case
for the static function names?

~~~

TupleTableSlotToJsonDatum:

13.
+ * This would be a new internal helper function for logical replication
+ * Needs to handle various data types and potentially TOASTed data

What's this comment about? Something doesn't look quite right.

~~~

InsertConflictLog:

14.
+ /* TODO: proper error code */
+ relid = get_relname_relid(relname, nspid);
+ if (!OidIsValid(relid))
+ elog(ERROR, "conflict log history table does not exists");
+ conflictrel = table_open(relid, RowExclusiveLock);
+ if (conflictrel == NULL)
+ elog(ERROR, "could not open conflict log history table");

14a.
What's the TODO comment for? Are you going to replace these elogs?

~

14b.
Typo: "does not exists"

~

14c.
An unnecessary double-blank line follows this code fragment.

~~~

15.
+ /* Populate the values and nulls arrays */
+ attno = 0;
+ values[attno] = ObjectIdGetDatum(RelationGetRelid(rel));
+ attno++;
+
+ if (TransactionIdIsValid(local_xid))
+ values[attno] = TransactionIdGetDatum(local_xid);
+ else
+ nulls[attno] = true;
+ attno++;
+
+ if (TransactionIdIsValid(remote_xid))
+ values[attno] = TransactionIdGetDatum(remote_xid);
+ else
+ nulls[attno] = true;
+ attno++;
+
+ values[attno] = LSNGetDatum(remote_final_lsn);
+ attno++;
+
+ if (local_ts > 0)
+ values[attno] = TimestampTzGetDatum(local_ts);
+ else
+ nulls[attno] = true;
+ attno++;
+
+ if (remote_commit_ts > 0)
+ values[attno] = TimestampTzGetDatum(remote_commit_ts);
+ else
+ nulls[attno] = true;
+ attno++;
+
+ values[attno] =
+ CStringGetTextDatum(get_namespace_name(RelationGetNamespace(rel)));
+ attno++;
+
+ values[attno] = CStringGetTextDatum(RelationGetRelationName(rel));
+ attno++;
+
+ values[attno] = CStringGetTextDatum(ConflictTypeNames[conflict_type]);
+ attno++;
+
+ if (origin_id != InvalidRepOriginId)
+ replorigin_by_oid(origin_id, true, &origin);
+
+ if (origin != NULL)
+ values[attno] = CStringGetTextDatum(origin);
+ else
+ nulls[attno] = true;
+ attno++;
+
+ if (replorigin_session_origin != InvalidRepOriginId)
+ replorigin_by_oid(replorigin_session_origin, true, &remote_origin);
+
+ if (remote_origin != NULL)
+ values[attno] = CStringGetTextDatum(remote_origin);
+ else
+ nulls[attno] = true;
+ attno++;
+
+ if (searchslot != NULL)
+ values[attno] = TupleTableSlotToJsonDatum(searchslot);
+ else
+ nulls[attno] = true;
+ attno++;
+
+ if (localslot != NULL)
+ values[attno] = TupleTableSlotToJsonDatum(localslot);
+ else
+ nulls[attno] = true;
+ attno++;
+
+ if (remoteslot != NULL)
+ values[attno] = TupleTableSlotToJsonDatum(remoteslot);
+ else
+ nulls[attno] = true;
+

15a.
It might be simpler to just post-increment that 'attno' in all the
assignments and save a dozen lines of code:
e.g. values[attno++] = ...

~

15b.
Also, put a sanity Assert check at the end, like:
Assert(attno + 1 == MAX_CONFLICT_ATTR_NUM);

======
src/backend/utils/cache/lsyscache.c

16.
+ if (isnull)
+ {
+ ReleaseSysCache(tup);
+ return NULL;
+ }
+
+ *nspid = subform->subconflictnspid;
+ relname = pstrdup(TextDatumGetCString(datum));
+
+ ReleaseSysCache(tup);
+
+ return relname;

It would be tidier to have a single release/return by coding this
slightly differently.

SUGGESTION:

char *relname = NULL;
...
if (!isnull)
{
*nspid = subform->subconflictnspid;
relname = pstrdup(TextDatumGetCString(datum));
}

ReleaseSysCache(tup);
return relname;

======
src/include/catalog/pg_subscription.h

17.
+ Oid subconflictnspid; /* Namespace Oid in which the conflict history
+ * table is created. */

Would it be better to make these 2 new member names more alike, since
they go together. e.g.
confl_table_nspid
confl_table_name

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

18.
+#define MAX_CONFLICT_ATTR_NUM 15

I felt this doesn't really belong here. Just define it atop/within the
function InsertConflictLog()

~~~

19.
extern void InitConflictIndexes(ResultRelInfo *relInfo);
+
#endif

Spurious whitespace change not needed for this patch.

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

20.
How about adding some more test scenarios:
e.g.1. ALTER the conflict log table of some subscription that already has one
e.g.2. Have multiple subscriptions that specify the same conflict log table

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