Re: Logical Replication of sequences

On Thu, Jul 17, 2025 at 4:52 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Wed, 16 Jul 2025 at 11:15, Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> > 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);
>
> Modified
>
> > 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'
>
> Modified
>
> > 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.
>
> Updated commit message
>
> The attached v20250717 version patch has the changes for the same.

Thanks, 0001 looks fine after this, I will share the feedback for other patches.


--
Regards,
Dilip Kumar
Google