Re: Logical Replication of sequences - Mailing list pgsql-hackers

Please find few initial comments for 002:


1)
Patch commit msg says:

"This patch adds support for a new SQL command:
ALTER SUBSCRIPTION ... REFRESH SEQUENCES
This command update the sequence entries present in the
pg_subscription_rel catalog table with the INIT state to trigger
resynchronization."

But AlterSubscription_refresh_seq actually updates the state to DATASYNC.

2)
CheckSubscriptionRelkind()

+ /*
+ * Allow RELKIND_RELATION and RELKIND_PARTITIONED_TABLE to be treated
+ * interchangeably, but ensure that sequences (RELKIND_SEQUENCE) match
+ * exactly on both publisher and subscriber.
+ */
+ if ((relkind == RELKIND_SEQUENCE && pubrelkind != RELKIND_SEQUENCE) ||
+ ((relkind == RELKIND_RELATION || relkind == RELKIND_PARTITIONED_TABLE) &&
+ !(pubrelkind == RELKIND_RELATION || pubrelkind == RELKIND_PARTITIONED_TABLE)))
+ ereport(ERROR,
+ errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("relation \"%s.%s\" type mismatch: source \"%s\", target \"%s\"",
+    nspname, relname,
+    pubrelkind == RELKIND_SEQUENCE ? "sequence" : "table",
+    relkind == RELKIND_SEQUENCE ? "sequence" : "table"));

Shall we simplify the check as:
if ((relkind == RELKIND_SEQUENCE && pubrelkind != RELKIND_SEQUENCE) ||
     (relkind != RELKIND_SEQUENCE && pubrelkind == RELKIND_SEQUENCE))

3)
CreateSubscription()
+ relations = fetch_relation_list(wrconn, publications);

Can we please rename 'relations' to 'pubrels', as the latter gives
more clarity and is in consistency with AlterSubscription_refresh().

4)
- CheckSubscriptionRelkind(get_rel_relkind(relid),
+ CheckSubscriptionRelkind(relkind, relinfo->relkind,
  rv->schemaname, rv->relname);

We are passing 2 relkinds now to CheckSubscriptionRelkind() but it is
difficult to understand which is which. Can we please rename relinfo
as pubrelinfo so that we get clarity. This is in both
CreateSubscription and AlterSubscription_refresh/

5)
CheckSubscriptionRelkind
+ if (pubrelkind == '\0')
+ return;

Do you think, we shall write a comment in the function header that the
caller who wants to verify only the supported type should pass
pubrelkind as '\0'?

6)
Should we update doc of pg_subscription_rel where it says this:

This catalog only contains tables known to the subscription after
running either CREATE SUBSCRIPTION or ALTER SUBSCRIPTION ... REFRESH
PUBLICATION.

thanks
Shveta