Re: Logical Replication of sequences

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.

Regards,
Vignesh