Re: Logical Replication of sequences

On Thu, Sep 18, 2025 at 4:07 PM vignesh C <vignesh21@gmail.com> wrote:
>
> Thanks for the comments, these are handled in the attached patch.
>

Please find a few comments:


patch005:
1)
GetSubscriptionRelations:
+ /* Skip sequences if they were not requested */
+ if (!get_sequences && (relkind == RELKIND_SEQUENCE))
+ continue;
+
+ /* Skip tables if they were not requested */
+ if (!get_tables && (relkind != RELKIND_SEQUENCE))
+ continue;

The use of negated conditions makes the logic harder to follow,
especially in the second if block.

Can we write it as:
   bool is_sequence = (relkind == RELKIND_SEQUENCE);

   /* Skip if the relation type is not requested */
   if ((get_tables && is_sequence) ||
       (get_sequences && !is_sequence))
       continue;

Or at-least:
/* Skip sequences if they were not requested */
if (get_tables && (relkind == RELKIND_SEQUENCE))
  continue;

/* Skip tables if they were not requested */
if (get_sequences && (relkind != RELKIND_SEQUENCE))
  continue;

2)

AlterSubscription_refresh_seq:

+ UpdateSubscriptionRelState(sub->oid, relid, SUBREL_STATE_DATASYNC,
+    InvalidXLogRecPtr, false);

Now it seems we are setting SUBREL_STATE_DATASYNC state as well for
sequences. Earlier it was INIT only.

So we need correction at 2 places:
a)
Comment atop GetSubscriptionRelations() which mentions :
 * If getting sequences and not_ready is false, retrieve all sequences;
 * otherwise, retrieve only sequences that are still in the INIT state
 * (i.e., have not reached the READY state).

We shall change it to that have not reached the READY state.

b)
patch 006's commit message says:
This patch introduces sequence synchronization:
Sequences have 2 states:
   - INIT (needs synchronizing)
   - READY (is already synchronized)

We shall mention the third state as well.


3)
There is some optimization in fetch_relation_list() in patch006. I
think it should be in patch005 itself where we have4 added new logic
to fetch sequeneces and relkind in patch005.
Or do we need those for patch006 specifically?


patch006:
4)
sequencesync.c:

+ * Sequences to be synchronized by the sequencesync worker will
+ * be added to pg_subscription_rel in INIT state when one of the following
+ * commands is executed:
+ * CREATE SUBSCRIPTION
+ * ALTER SUBSCRIPTION ... REFRESH PUBLICATION
+ * ALTER SUBSCRIPTION ... REFRESH PUBLICATION SEQUENCES

I think this comment needs change. 'REFRESH PUBLICATION SEQUENCES' is
not doing that anymore.

5)
 * So the state progression is always just: INIT -> READY.
I see even SUBREL_STATE_DATASYNC being set now by
AlterSubscription_refresh_seq()

thanks
Shveta