Re: Logical Replication of sequences

On Fri, 4 Oct 2024 at 15:39, shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
>
> On Sun, Sep 29, 2024 at 12:34 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> >
> > On Thu, 26 Sept 2024 at 11:07, shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
> > >
> > > On Fri, Sep 20, 2024 at 9:36 AM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> > > >
> > > > On Wed, 21 Aug 2024 at 11:54, vignesh C <vignesh21(at)gmail(dot)com> wrote:
> > > > >
> > > > > On Wed, 21 Aug 2024 at 08:33, Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> > > > > >
> > > > > > Hi Vignesh, Here are my only review comments for the latest patch set.
> > > > >
> > > > > Thanks, these issues have been addressed in the updated version.
> > > > > Additionally, I have fixed the pgindent problems that were reported
> > > > > and included another advantage of this design in the file header of
> > > > > the sequencesync file.
> > > >
> > > > The patch was not applied on top of head, here is a rebased version of
> > > > the patches.
> > > > I have also removed an invalidation which was not required for
> > > > sequences and a typo.
> > > >
> > >
> > > Thank You for the patches. I would like to understand srsublsn and
> > > page_lsn more. Please see the scenario below:
> > >
> > > I have a sequence:
> > > CREATE SEQUENCE myseq0 INCREMENT 5 START 100;
> > >
> > > After refresh on sub:
> > > postgres=# ALTER SUBSCRIPTION sub1 REFRESH PUBLICATION SEQUENCES;
> > > ALTER SUBSCRIPTION
> > >
> > > postgres=# select * from pg_subscription_rel;
> > > srsubid | srrelid | srsubstate | srsublsn
> > > ---------+---------+------------+-----------
> > > 16385 | 16384 | r | 0/152F380 -->pub's page_lsn
> > >
> > >
> > > postgres=# select * from pg_sequence_state('myseq0');
> > > page_lsn | last_value | log_cnt | is_called
> > > -----------+------------+---------+-----------
> > > 0/152D830 | 105 | 31 | t -->(I am assuming 0/152D830 is
> > > local page_lsn corresponding to value-=105)
> > >
> > > Now I assume that *only* after doing next_wal for 31 times, page_lsn
> > > shall change. But I observe strange behaviour
> > >
> > > After running nextval on sub for 7 times:
> > > postgres=# select * from pg_sequence_state('myseq0');
> > > page_lsn | last_value | log_cnt | is_called
> > > -----------+------------+---------+-----------
> > > 0/152D830 | 140 | 24 | t -->correct
> > >
> > > After running nextval on sub for 15 more times:
> > > postgres=# select * from pg_sequence_state('myseq0');
> > > page_lsn | last_value | log_cnt | is_called
> > > -----------+------------+---------+-----------
> > > 0/152D830 | 215 | 9 | t -->correct
> > > (1 row)
> > >
> > > Now after running it 6 more times:
> > > postgres=# select * from pg_sequence_state('myseq0');
> > > page_lsn | last_value | log_cnt | is_called
> > > -----------+------------+---------+-----------
> > > 0/152D990 | 245 | 28 | t --> how??
> > >
> > > last_value increased in the expected way (6*5), but page_lsn changed
> > > and log_cnt changed before we could complete the remaining runs as
> > > well. Not sure why??
> >
> > This can occur if a checkpoint happened at that time. The regression
> > test also has specific handling for this, as noted in a comment within
> > the sequence.sql test file:
> > -- log_cnt can be higher if there is a checkpoint just at the right
> > -- time
>
> Okay. I see. I tried by executing 'checkpoint' and can see the same behaviour.
>
> >
> > > Now if I do refresh again:
> > >
> > > postgres=# ALTER SUBSCRIPTION sub1 REFRESH PUBLICATION SEQUENCES;
> > > ALTER SUBSCRIPTION
> > >
> > > postgres=# select * from pg_subscription_rel;
> > > srsubid | srrelid | srsubstate | srsublsn
> > > ---------+---------+------------+-----------
> > > 16385 | 16384 | r | 0/152F380-->pub's page_lsn, same as old one.
> > >
> > > postgres=# select * from pg_sequence_state('myseq0');
> > > page_lsn | last_value | log_cnt | is_called
> > > -----------+------------+---------+-----------
> > > 0/152DDB8 | 105 | 31 | t
> > > (1 row)
> > >
> > > Now, what is this page_lsn = 0/152DDB8? Should it be the one
> > > corresponding to last_value=105 and thus shouldn't it match the
> > > previous value of 0/152D830?
> >
> > After executing REFRESH PUBLICATION SEQUENCES, the publication value
> > will be resynchronized, and a new LSN will be generated and updated
> > for the publisher sequence (using the old value). Therefore, this is
> > not a concern.
> >
>
> Okay.
>
> Few comments:
>
> 1)
> +static List *
> +fetch_sequence_list(WalReceiverConn *wrconn, char *subname, List *publications)
>
> --fetch_sequence_list() is not using the argument subanme anywhere.
>
> 2)
>
> + if (resync_all_sequences)
> + {
> + ereport(DEBUG1,
> + errmsg_internal("sequence \"%s.%s\" of subscription \"%s\" set to INIT state",
> + get_namespace_name(get_rel_namespace(relid)),
> + get_rel_name(relid),
> + sub->name));
> + UpdateSubscriptionRelState(sub->oid, relid, SUBREL_STATE_INIT,
> + InvalidXLogRecPtr);
> + }
>
> --Shall we have DEBUG1 after we are done with
> UpdateSubscriptionRelState? Otherwise we may end up putting this log
> statement, even if the update fails for some reason.
>
> 3)
> fetch_remote_sequence_data():
>
> Should we have a macro REMOTE_SEQ_COL_COUNT 10 and use it instead of
> direct 10. Also instead of having 1,2,3 etc in slot_getattr, we can
> have ++col and at the end we can have:
> Assert(col == REMOTE_SEQ_COL_COUNT);

Thanks for the comments, these are addressed in the attached patch.

Regards,
Vignesh