Re: Logical Replication of sequences

> On Oct 17, 2025, at 17:34, Chao Li <li.evan.chao@gmail.com> wrote:
>
> I may find some time to review 0002 and 0003 next week.

I just reviewed 0002 and 0003. Got some comments for 0002, and no comment for 0003.


1 - 0002 - commit comment
```
Sequences have 3 states:
   - INIT (needs [re]synchronizing)
   - READY (is already synchronized)
```

3 states or 2 states? Missing DATASYNC?


2 - 0002 - launcher.c
```
+ * For both apply workers and sequence sync workers, the relid should be set to
+ * InvalidOid, as they manage changes across all tables and sequences. For table
+ * sync workers, the relid should be set to the OID of the relation being
+ * synchronized.
```

Nit: “both” sounds not necessarily.

3 - 0002 - launcher.c
```
  * Stop the logical replication worker for subid/relid, if any.
  */
 void
-logicalrep_worker_stop(Oid subid, Oid relid)
+logicalrep_worker_stop(Oid subid, Oid relid, LogicalRepWorkerType wtype)
```

Should the comment be updated? “subid/relid/wtype"

4 - 0002 - launcher.c
```
-    worker = logicalrep_worker_find(subid, relid, true);
+    worker = logicalrep_worker_find(subid, relid, WORKERTYPE_APPLY, true);
```

Based the comment you added to “logicalrep_worker_find()”, for apply worker, relid should be set to InvalidOid. (See
comment2) 

Then if you change this function to only work for WORKERTYPE_APPLY, relid should be hard code to InvalidOid, so that
“relid”can be removed from parameter list of logicalrep_worker_wakeup(). 

5 - 0002 - launcher.c
```
/*
* report_error_sequences
*
* Reports discrepancies in sequence data between the publisher and subscriber.
```

Nit: “Reports” -> “Report”

Here “reports” also work. But looking at the previous function’s comment:

```
 * Handle sequence synchronization cooperation from the apply worker.
*
* Start a sequencesync worker if one is not already running. The active
```

You used “handle” and “start” but “handles” and “starts”.

“Report” better matches imperative style in PG comments.

6 - 0002 - sequencesync.c
```
+report_error_sequences(StringInfo insuffperm_seqs, StringInfo mismatched_seqs)
+{
+    StringInfo    combined_error_detail = makeStringInfo();
+    StringInfo    combined_error_hint = makeStringInfo();
```

By the end of this function, I think we need to destroyStringInfo() these two strings.

7 - 0002 - sequencesync.c
```
+static void
+append_sequence_name(StringInfo buf, const char *nspname, const char *seqname,
+                     int *count)
+{
+    if (buf->len > 0)
+        appendStringInfoString(buf, ", “);
```

Why this “if” check is needed?

8 - 0002 - sequencesync.c
```
+        destroyStringInfo(seqstr);
+        destroyStringInfo(cmd);
```

Instead of making and destroying seqstr and cmd in every iteration, we can create them before “while”, and only
resetStringInfo()them for every iteration, which should be cheaper and faster. 

9 - 0002 - sequencesync.c
```
+    return h1 ^ h2;
+    /* XOR combine */
+}
```

This code is very descriptive, so the commend looks redundant. Also, why put the comment below the code line?

10 - 0002 - syncutils.c
```
 /*
- * Common code to fetch the up-to-date sync state info into the static lists.
+ * Common code to fetch the up-to-date sync state info for tables and sequences.
  *
- * Returns true if subscription has 1 or more tables, else false.
+ * The pg_subscription_rel catalog is shared by tables and sequences. Changes
+ * to either sequences or tables can affect the validity of relation states, so
+ * we identify non-ready tables and non-ready sequences together to ensure
+ * consistency.
  *
- * Note: If this function started the transaction (indicated by the parameter)
- * then it is the caller's responsibility to commit it.
+ * Returns true if subscription has 1 or more tables, else false.
  */
 bool
-FetchRelationStates(bool *started_tx)
+FetchRelationStates(bool *has_pending_sequences, bool *started_tx)
```
has_pending_sequences is not explained in the function comment.

11 - 0002 - syncutils.c
```
    /*
* has_subtables and has_subsequences is declared as static, since the
* same value can be used until the system table is invalidated.
*/
static bool has_subtables = false;
static bool has_subsequences_non_ready = false;
```

The comment says “has_subsequences”, but the real var name is “has_subsequences_non_ready".

12 - 0002 - syncutils.c
```
 bool
-FetchRelationStates(bool *started_tx)
+FetchRelationStates(bool *has_pending_sequences, bool *started_tx)
```

I searched over the code, this function has 3 callers, none of them want results for both table and sequence, so that a
caller,for example: 

```
bool
HasSubscriptionTablesCached(void)
{
bool started_tx;
bool has_subrels;
bool has_pending_sequences;

/* We need up-to-date subscription tables info here */
has_subrels = FetchRelationStates(&has_pending_sequences, &started_tx);

if (started_tx)
{
CommitTransactionCommand();
pgstat_report_stat(true);
}

return has_subrels;
}
```

Looks confused because it defines has_pending_sequences but not using it at all.

So, I think FetchRelationStates() can be refactored to return a result for either table or sequence, because on a type
parameter.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/