Re: Logical Replication of sequences

On Fri, 15 Aug 2025 at 16:46, Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.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