Re: Proposal: Conflict log history table for Logical Replication

Some review comments for patch v45-0003.

======
src/backend/catalog/heap.c

heap_create:

1.
+ * Allow creation of conflict table in binary-upgrade mode.

/of conflict table/of the conflict log table/

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

alter_sub_conflictlogdestination:

2.
+ char relname[NAMEDATALEN];

- relid = create_conflict_log_table(sub->oid, sub->name, sub->owner);
- update_relid = true;
+ snprintf(relname, NAMEDATALEN, "pg_conflict_log_%u", sub->oid);

Currently, we are generating the CLT name here and also in
create_conflict_log_table. I think there should be only *one* place
that does the CLT name generation. e.g. suggest making a new common
function like get_conflict_log_table_name(sub->oid, &relname,
NAMEDATALEN);

~~~

3.
+ relid = get_relname_relid(relname, PG_CONFLICT_NAMESPACE);
+ if (OidIsValid(relid))
+ {
+ /* Existing table from upgrade */
+ Assert(IsBinaryUpgrade);
+ }
+ else

Should this be the other way around?

if (IsBinaryUpgrade)
{
relid = get_relname_relid(relname, PG_CONFLICT_NAMESPACE);
Assert(OidIsValid(relid));
}

~~~

4.
+ /*
+ * Establish an internal dependency between the conflict log
+ * table and the subscription. For details refer comments in
* CreateSubscription function.
*/

SUGGESTION
Refer to comments in the CreateSubscription function for details.

======
src/bin/pg_dump/pg_dump.c

selectDumpableTable:

5.
+ if (strcmp(tbinfo->dobj.namespace->dobj.name, "pg_conflict") == 0)
+ {
+ if (dopt->binary_upgrade)
+ {
+ /*
+ * Dump pg_conflict tables only during binary upgrade. The schema
+ * is assumed to already exist.
+ */
+ tbinfo->dobj.dump = DUMP_COMPONENT_DEFINITION;
+
+ /*
+ * Suppress the "ALTER TABLE ... OWNER TO ..." command for this
+ * table. This prevents pg_dump from outputting the owner change.
+ */
+ tbinfo->rolname = NULL;
+ }
+ else
+ tbinfo->dobj.dump = DUMP_COMPONENT_NONE;

Doesn't the comment "Dump pg_conflict tables only during binary
upgrade..." really belong above the "if (dopt->binary_upgrade)".

~~~

6.
+ if (fout->remoteVersion >= 190000)
+ appendPQExpBufferStr(query,
+ " s.subconflictlogrelid, s.subconflictlogdest\n");
else
- appendPQExpBufferStr(query, " NULL AS subservername\n");
+ appendPQExpBufferStr(query,
+ " NULL AS subconflictlogrelid, NULL AS subconflictlogdest\n");

It seems more natural to think of the 'subconflictlogdest' before the
subconflictlogrelid. Perhaps the SQL should swap those around.

~~~

7.
+ if (PQgetisnull(res, i, i_subconflictlogrelid))
+ subinfo[i].subconflictlogrelid = InvalidOid;
+ else
+ {
+ TableInfo *tableInfo;
+
+ subinfo[i].subconflictlogrelid =
+ atooid(PQgetvalue(res, i, i_subconflictlogrelid));
+
+ if (subinfo[i].subconflictlogrelid)
+ {
+ tableInfo = findTableByOid(subinfo[i].subconflictlogrelid);
+ if (!tableInfo)
+ pg_fatal("could not find conflict log table with OID %u",
+ subinfo[i].subconflictlogrelid);
+
+ addObjectDependency(&subinfo[i].dobj, tableInfo->dobj.dumpId);
+ }
+ }
+
+ if (PQgetisnull(res, i, i_sublogdestination))
+ subinfo[i].subconflictlogdest = NULL;
+ else
+ subinfo[i].subconflictlogdest =
+ pg_strdup(PQgetvalue(res, i, i_sublogdestination));
+

7a.
Those new attributes come as a pair -- e.g. the relid only makes sense
depending on the destination value; maybe only look for the CLT relid
is the destination is not LOG/NULL

Knowing the destination also means you can also do integrity checks
for PQgetisnull(res, i, i_subconflictlogrelid) -- e.g. must be valid
Oid, or must be invalid Oid.

~

7b.
The 'tableInfo' can be declared/assigned in the if block.

~~~

dumpSubscription:

8.
+ if (subinfo->subconflictlogdest &&
+ (pg_strcasecmp(subinfo->subconflictlogdest, "log") != 0))
+ appendPQExpBuffer(query,
+ "\n\nALTER SUBSCRIPTION %s SET (conflict_log_destination = %s);\n",
+ qsubname,
+ subinfo->subconflictlogdest);
+

8a.
Why was this implemented as a separate ALTER SUBSCRIPTION instead of
just another CREATE SUBSCRIPTION option like all the others? If there
is some good reason, these should be a comment here.

~~~

8b.
Not sure why this has a double \n\n prefix instead of just \n. I
didn't see that pattern elsewhere in this file.

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