Re: Logical Replication of sequences

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
Shveta