Re: Logical Replication of sequences - Mailing list pgsql-hackers

On Sun, Sep 29, 2024 at 12:34 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Thu, 26 Sept 2024 at 11:07, shveta malik <shveta.malik@gmail.com> wrote:
> >
> > On Fri, Sep 20, 2024 at 9:36 AM vignesh C <vignesh21@gmail.com> wrote:
> > >
> > > On Wed, 21 Aug 2024 at 11:54, vignesh C <vignesh21@gmail.com> wrote:
> > > >
> > > > On Wed, 21 Aug 2024 at 08:33, Peter Smith <smithpb2250@gmail.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