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

Hi Vignesh,

Here are a few review comments for patch 0001:

======
src/backend/catalog/pg_subscription.c

GetSubscriptionRelations:

1.
 List *
-GetSubscriptionRelations(Oid subid, bool not_ready)
+GetSubscriptionRelations(Oid subid, bool otherwise return all the
relations of the subscription, bool get_sequences,
+ bool not_ready)

Now the function parameter means the (unchanged) function comment is
not correct anymore.

e.g. It still says "otherwise return all the relations of the
subscription", but that does not account for the parameters indicating
if you only want tables, or only want sequences.

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

2.
+typedef struct PublicationRelKind
+{
+ RangeVar   *rv;
+ char relkind;
+} PublicationRelKind;
+

Is this deserving of a comment to saying what it is for?

~~~

CreateSubscription:

3.
+ *
+ * Similar to origins, it is not clear whether preventing the slot
+ * creation for empty and sequence-only subscriptions is worth
+ * additional complexity.
  */

I think this "Similar to..." comment needs "XXX", same as the earlier
comment it is referring to.

~~~

AlterSubscription_refresh:

4.
+ * Build qsorted array of local relation oids for faster lookup. This
+ * can potentially contain all relation in the database so speed of
+ * lookup is important.
+ *

/all relation/all relations/

~~~

5.
- qsort(subrel_local_oids, subrel_count,
+ qsort(subrel_local_oids, tbl_count,
    sizeof(Oid), oid_cmp);

- check_publications_origin(wrconn, sub->publications, copy_data,
-   sub->retaindeadtuples, sub->origin,
-   subrel_local_oids, subrel_count, sub->name);
+ check_publications_origin_tables(wrconn, sub->publications, copy_data,
+ sub->retaindeadtuples, sub->origin,
+ subrel_local_oids, tbl_count,
+ sub->name);

- /*
- * Rels that we want to remove from subscription and drop any slots
- * and origins corresponding to them.
- */
- sub_remove_rels = palloc(subrel_count * sizeof(SubRemoveRels));
+ qsort(subseq_local_oids, seq_count, sizeof(Oid), oid_cmp);
+ check_publications_origin_sequences(wrconn, sub->publications,
+ sub->origin, subseq_local_oids,
+ seq_count, sub->name);

The first qsort wrapping and the blank line spacing are a bit inconsistent here.

~~~

6.
+static void
+check_publications_origin_sequences(WalReceiverConn *wrconn, List
*publications,
+ char *origin, Oid *subrel_local_oids,
+ int subrel_count, char *subname)

Deserving of a function comment?

~

7.
+ for (i = 0; i < subrel_count; i++)
+ {

Declare 'i' as a for-loop variable.

~

8.
+ if (res->status != WALRCV_OK_TUPLES)
+ ereport(ERROR,
+ (errcode(ERRCODE_CONNECTION_FAILURE),
+ errmsg("could not receive list of replicated relations from the
publisher: %s",
+ res->err)));
+
+ /* Process relations. */
+ slot = MakeSingleTupleTableSlot(res->tupledesc, &TTSOpsMinimalTuple);
+ while (tuplestore_gettupleslot(res->tuplestore, true, false, slot))

Since this function is just for sequences, should it say "replicated
sequences" instead of "relations" here?

Similarly, should the "Process relations" maybe say "Process sequence
relations" or "Process sequences"?

~~~

fetch_table_list:

9.
+ appendStringInfo(&cmd,
+ "UNION ALL\n"
+ "  SELECT DISTINCT s.schemaname, s.sequencename, NULL::int2vector AS
attrs, " CppAsString2(RELKIND_SEQUENCE) "::\"char\" AS relkind\n"
+ "  FROM pg_catalog.pg_publication_sequences s\n"
+ "  WHERE s.pubname IN (%s)",
+ pub_names->data);

Missing a final \n?

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

10.
bool
FetchRelationStates(bool *started_tx)

IIUC, you are using the terminology "relations" to apply to both
tables or sequences; OTOH ", tables" is just tables. It seems this
function is table-specific. Should it have a name change like
FetchTableStates?

======
src/test/subscription/t/036_sequences.pl

11.
There are no test cases checking that ALTER commands will correctly
add/remove sequences in the pg_subscription_rel?

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