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.
Regards,
Vignesh
