On Thu, 1 Aug 2024 at 09:26, shveta malik <shveta.malik@gmail.com> wrote:
>
> On Mon, Jul 29, 2024 at 4:17 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > Thanks for reporting this, these issues are fixed in the attached
> > v20240730_2 version patch.
> >
>
> Thanks for addressing the comments. Please find few comments on patch001 alone:
>
> Potential Bug:
> 1) 'last_value' returned by pg_sequence_state() is wrong initially.
>
> postgres=# create sequence myseq5;
> CREATE SEQUENCE
>
> postgres=# select * from pg_sequence_state('myseq5');
> page_lsn | last_value | log_cnt | is_called
> -----------+------------+---------+-----------
> 0/1579C78 | 1 | 0 | f
>
> postgres=# SELECT nextval('myseq5') ;
> nextval
> ---------
> 1
>
> postgres=# select * from pg_sequence_state('myseq5');
> page_lsn | last_value | log_cnt | is_called
> -----------+------------+---------+-----------
> 0/1579FD8 | 1 | 32 | t
>
>
> Both calls returned 1. First call should have returned NULL.
I noticed the same behavior for selecting from a sequence:
postgres=# select * from myseq5;
last_value | log_cnt | is_called
------------+---------+-----------
1 | 0 | f
(1 row)
postgres=# select nextval('myseq5');
nextval
---------
1
(1 row)
postgres=# select * from myseq5;
last_value | log_cnt | is_called
------------+---------+-----------
1 | 32 | t
(1 row)
By default it shows the last_value as the start value for the
sequence. So this looks ok to me.
> 2)
> func.sgml:
> a) pg_sequence_state : Don't we need to give input arg as regclass
> like we give in nextval,setval etc?
> b) Should 'lsn' be changed to 'page_lsn' as returned in output of
> pg_sequence_state()
Modified
>
> 3)
> read_seq_tuple() header says:
> * lsn_ret will be set to the page LSN if the caller requested it.
> * This allows the caller to determine which sequence changes are
> * before/after the returned sequence state.
>
> How, using lsn which is page-lsn and not sequence value/change lsn,
> does the user interpret if sequence changes are before/after the
> returned sequence state? Can you please elaborate or amend the
> comment?
I have added this to pg_sequence_state function header in 003 patch as
the subscriber side changes are present here, I felt that is more apt
to mention. This is also added in sequencesync.c file header.
The attached v20240805_2 version patch has the changes for the same.
Regards,
Vignesh
