Re: Logical Replication of sequences

On Thu, Sep 18, 2025 at 4:07 PM vignesh C <vignesh21(at)gmail(dot)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