Re: Logical Replication of sequences

On Thu, 31 Oct 2024 at 14:57, vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> On Thu, 24 Oct 2024 at 04:24, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)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