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
