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
