Re: Logical Replication of sequences

On Fri, 15 Aug 2025 at 16:46, Hayato Kuroda (Fujitsu)
<kuroda(dot)hayato(at)fujitsu(dot)com> wrote:
>
> Dear Vignesh,
>
> Thanks for updating the patch. Here are my small comments:
>
> 01.
> Per pgindent report, publicationcmds.c should be fixed.

Modified

> 02.
> ```
> + ScanKeyInit(&skey[1],
> + Anum_pg_subscription_rel_srsubstate,
> + BTEqualStrategyNumber, F_CHARNE,
> + CharGetDatum(SUBREL_STATE_READY));
> ```
>
> I felt it is more natural to "srsubstate = 'i'", instead of "srsubstate <> 'r'"

Modified

> 03.
> ```
> + table_close(sequence_rel, NoLock);
> + }
> +
> + /* Cleanup */
> + systable_endscan(scan);
> + table_close(rel, AccessShareLock);
> +
> + CommitTransactionCommand();
> ```
>
> To clarify, can we release the sequence at the end of the inner loop?
>
> I found that sequence relation is closed (but not release the lock) then commit
> the transaction once. This approach cannot avoid dropping it by concurrent
> transactions, but maybe you did due to the performance reason. So...I felt we
> may able to release bit earlier.

Modified

> 04.
> ```
> + sequence_rel = try_table_open(seqinfo->localrelid, RowExclusiveLock);
> +
> + /* Get the local sequence */
> + tup = SearchSysCache1(SEQRELID, ObjectIdGetDatum(seqinfo->localrelid));
> + if (!sequence_rel || !HeapTupleIsValid(tup))
> + {
> + elog(LOG, "skip synchronization of sequence \"%s.%s\" because it has been dropped concurrently",
> + seqinfo->nspname, seqinfo->seqname);
> +
> + batch_skipped_count++;
> + continue;
> + }
> ```
>
> a. Code comment can be atop try_table_open().
> b. Isn't it enough to check HeapTupleIsValid() here?

Modified

> 05.
> ```
> + /* Update the sequence only if the parameters are identical */
> + if (seqform->seqtypid == seqtypid &&
> + seqform->seqmin == seqmin && seqform->seqmax == seqmax &&
> + seqform->seqcycle == seqcycle &&
> + seqform->seqstart == seqstart &&
> + seqform->seqincrement == seqincrement)
> ```
>
> I noticed that seqcache is not compared. Is there a reason?

I felt we could go ahead and set the sequence value even if seqcache
is different unlike the other sequence parameters. That is the reason
I did not compare it. Thoughts?

> 06.
> ```
> + foreach_ptr(LogicalRepSequenceInfo, seq_info, sequences_to_copy)
> + {
> + pfree(seq_info->seqname);
> + pfree(seq_info->nspname);
> + pfree(seq_info);
> + }
> ```
>
> Per comment atop foreach_delete_current(), we should not directly do pfree()
> the entry. Can you use foreach_delete_current()? I.e.,

Modified

> 07.
> ```
> foreach_ptr(LogicalRepSequenceInfo, seq_info, sequences_to_copy)
> {
> pfree(seq_info->seqname);
> pfree(seq_info->nspname);
>
> sequences_to_copy =
> foreach_delete_current(sequences_to_copy, seq_info);
> }
> ```

Modified

> 08.
> ```
> +$node_subscriber->init(allows_streaming => 'logical');
> ```
>
> Actually no need to set to 'logical'.

Modified

Thanks for the comments, the updated version has the changes for the same.

Regards,
Vignesh