Re: Logical Replication of sequences

On Thu, 31 Oct 2024 at 14:57, vignesh C <vignesh21@gmail.com> wrote:
>
> On Thu, 24 Oct 2024 at 04:24, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > 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.
>
> Modified
>
> > 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.
>
> Modified
>
> > ---
> >         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.
>
> Modified
>
> > ---
> > 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?
>
> We are adding publication titles as footers to the sequence
> description, each with different publication names. Since the number
> of publications is unknown in advance, we will first determine the
> total number of publications, then allocate the necessary size for the
> footers. We will append footers[0] with either 'Owned by' or 'Sequence
> for identity column,' followed by the publication titles.
> Additionally, these will be subject to version checks. In this case,
> using printTable instead of printQuery is preferable, as it simplifies
> the code.
>
> The attached patch has the changes for the fixes.

The patch needed to be rebased; here is the updated version.

Regards,
Vignesh