Re: Proposal: Conflict log history table for Logical Replication

Here are some comments for the patch v4-0002.

======
GENERAL

1.
The patch should include test cases:

- to confirm an error happens when attempting to publish clt
- to confirm \dt+ clt is not showing the ALL TABLES publication
- to confirm that SQL function pg_relation_is_publishable givesthe
expected result
- etc.

======
Commit Message

1.
When all table option is used with publication don't publish the
conflict history tables.

~

Maybe reword that using uppercase for keywords, like:

SUGGESTION
A conflict log table will not be published by a FOR ALL TABLES publication.

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

check_publication_add_relation:

3.
+ /* Can't be created as 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.")));

3a.
Typo in comment.

SUGGESTION
Can't be a conflict log table

~

3b.
I was wondering if this check should be moved to the bottom of the function.

I think IsConflictLogRelid() is the most inefficient of all these
conditions, so it is better to give the other ones a chance to fail
quickly before needing to check for clt.

~~~

pg_relation_is_publishable:

4.
/*
- * SQL-callable variant of the above
+ * SQL-callable variant of the above and this should not be a conflict log rel
*
* This returns null when the relation does not exist. This is intended to be
* used for example in psql to avoid gratuitous errors when there are

I felt this new comment should be in the code, instead of in the
function comment.

SUGGESTION
/* subscription conflict log tables are not published */
result = is_publishable_class(relid, (Form_pg_class) GETSTRUCT(tuple)) &&
!IsConflictLogRelid(relid);

~~~

5.
It seemed strange that function
pg_relation_is_publishable(PG_FUNCTION_ARGS) is checking
IsConflictLogRelid, but function is_publishable_relation(Relation rel)
is not.

~~~

GetAllPublicationRelations:

6.
+ /* conflict history tables are not published. */
if (is_publishable_class(relid, relForm) &&
+ !IsConflictLogRelid(relid) &&
!(relForm->relispartition && pubviaroot))
result = lappend_oid(result, relid);
Inconsistent "history table" terminology.

Maybe this comment should be identical to the other one above. e.g.
/* subscription conflict log tables are not published */

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

IsConflictLogRelid:

8.
+/*
+ * Is relation used as a conflict log table
+ *
+ * Scan all the subscription and check whether the relation is used as
+ * conflict log table.
+ */

typo: "all the subscription"

Also, the 2nd sentence repeats the purpose of the function; I don't
think you need to say it twice.

SUGGESTION
Check if the specified relation is used as a conflict log table by any
subscription.

~~~

9.
+ if (relname == NULL)
+ continue;
+ if (relid == get_relname_relid(relname, nspid))
+ {
+ found = true;
+ break;
+ }

It seemed unnecessary to separate out the 'continue' like that.

In passing, consider renaming that generic 'found' to be the proper
meaning of the boolean.

SUGGESTION
if (relname && relid == get_relname_relid(relname, nspid))
{
is_clt = true;
break;
}

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