Horton

Re: Logical Replication of sequences

Dilip Kumar <dilipbalaut@gmail.com>

View thread
From: Dilip Kumar <dilipbalaut@gmail.com>
To: vignesh C <vignesh21@gmail.com>
Cc: shveta malik <shveta.malik@gmail.com>, Nisha Moond <nisha.moond412@gmail.com>, Amit Kapila <amit.kapila16@gmail.com>, Peter Smith <smithpb2250@gmail.com>, "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>, Masahiko Sawada <sawada.mshk@gmail.com>, Shlok Kyal <shlok.kyal.oss@gmail.com>, Peter Eisentraut <peter@eisentraut.org>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>, Euler Taveira <euler@eulerto.com>, Michael Paquier <michael@paquier.xyz>, "Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>, "Jonathan S. Katz" <jkatz@postgresql.org>
Date: 2025-07-16T05:44:44Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Doc: Add documentation for sequence synchronization.

  2. Remove unused assignment in CREATE PUBLICATION grammar.

  3. Add seq_sync_error_count to subscription statistics.

  4. Fix few issues in commit 5509055d69.

  5. Add sequence synchronization for logical replication.

  6. Add worker type argument to logical replication worker functions.

  7. Introduce "REFRESH SEQUENCES" for subscriptions.

  8. Refactor logical worker synchronization code into a separate file.

  9. Standardize use of REFRESH PUBLICATION in code and messages.

  10. Add "ALL SEQUENCES" support to publications.

  11. Expose sequence page LSN via pg_get_sequence_data.

  12. Resume conflict-relevant data retention automatically.

  13. Post-commit review fixes for 228c370868.

  14. Generate GUC tables from .dat file

On Mon, Jul 14, 2025 at 10:03 AM vignesh C <vignesh21@gmail.com> wrote:
>
> On Fri, 11 Jul 2025 at 14:26, shveta malik <shveta.malik@gmail.com> wrote:
> >
I have picked this up again for final review, started with 0001, I
think mostly 0001 looks good to me, except few comments

1.
+ lsn = PageGetLSN(page);
+ last_value = seq->last_value;
+ log_cnt = seq->log_cnt;
+ is_called = seq->is_called;
+
+ UnlockReleaseBuffer(buf);
+ sequence_close(seqrel, NoLock);
+
+ /* Page LSN for the sequence */
+ values[0] = LSNGetDatum(lsn);
+
+ /* The value most recently returned by nextval in the current session */
+ values[1] = Int64GetDatum(last_value);
+

I think we can avoid using extra variables like lsn, last_value etc
instead we can directly copy into the value[$] as shown below.

values[0] = LSNGetDatum(PageGetLSN(page));
values[1] = Int64GetDatum(seq->last_value);
...
UnlockReleaseBuffer(buf);
sequence_close(seqrel, NoLock);

2.
+       <para>
+        Returns information about the sequence. <literal>page_lsn</literal> is
+        the page LSN of the sequence, <literal>last_value</literal> is the
+        current value of the sequence, <literal>log_cnt</literal> shows how
+        many fetches remain before a new WAL record must be written, and
+        <literal>is_called</literal> indicates whether the sequence has been
+        used.

Shall we change 'is the page LSN of the sequence' to 'is the page LSN
of the sequence relation'

And I think this field doesn't seem to be very relevant for the user,
although we are exposing it because we need it for internal use.
Maybe at least the commit message of this patch should give some
details on why we need to expose this field.

-- 
Regards,
Dilip Kumar
Google