Re: Logical Replication of sequences

On Tue, Oct 8, 2024 at 2:46 AM vignesh C <vignesh21@gmail.com> wrote:
>
> On Fri, 4 Oct 2024 at 15:39, shveta malik <shveta.malik@gmail.com> wrote:
> >
> > 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 for the comments, these are addressed in the attached patch.
>

Here are comments on the 0001 and 0002 patches:

0001 patch:

read_seq_tuple() reads a buffer and acquires a lock on it, and the
buffer is returned to the caller while being locked. So I think it's
possible for the caller to get the page LSN even without changes.
Since pg_sequence_state() is the sole caller that requests lsn_ret to
be set, I think the changes of read_seq_tuples() is not necessarily
necessary.

0002 patch:
+        Assert(all_tables && *all_tables == false);
+        Assert(all_sequences && *all_sequences == false);

I think it's better to set both *all_tables and *all_sequence to false
at the beginning of the function to ensure this function works as
expected regardless of their initial values.

---
        appendPQExpBufferStr(query,
                             "SELECT p.tableoid, p.oid, p.pubname, "
                             "p.pubowner, "
-                            "p.puballtables, p.pubinsert,
p.pubupdate, p.pubdelete, p.pubtruncate, p.pubviaroot "
+                            "p.puballtables, false as
p.puballsequences, p.pubinsert, p.pubupdate, p.pubdelete,
p.pubtruncate, p.pubviaroot "
                             "FROM pg_publication p");
    else if (fout->remoteVersion >= 110000)
        appendPQExpBufferStr(query,
                             "SELECT p.tableoid, p.oid, p.pubname, "
                             "p.pubowner, "
-                            "p.puballtables, p.pubinsert,
p.pubupdate, p.pubdelete, p.pubtruncate, false AS pubviaroot "
+                            "p.puballtables, false as
p.puballsequences, p.pubinsert, p.pubupdate, p.pubdelete,
p.pubtruncate, false AS pubviaroot "
                             "FROM pg_publication p");
    else
        appendPQExpBufferStr(query,
                             "SELECT p.tableoid, p.oid, p.pubname, "
                             "p.pubowner, "
-                            "p.puballtables, p.pubinsert,
p.pubupdate, p.pubdelete, false AS pubtruncate, false AS pubviaroot "
+                            "p.puballtables, false as
p.puballsequences, p.pubinsert, p.pubupdate, p.pubdelete, false AS
pubtruncate, false AS pubviaroot "
                             "FROM pg_publication p");

The column name should be puballsequences, not p.puballsequences.

---
IIUC the changes of describeOneTableDetails() includes two kinds of
changes: refactoring to use printTable() instead of printQuery(), and
adding publications that includes the sequence. Is the first
refactoring necessary for the second change? If not, should it be done
in a separate patch?
fg
Regards,

-- 
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com