MIME-Version: 1.0
References: 
 <CAB-JLwbBFNuASyEnZWP0Tck9uNkthBZqi6WoXNevUT6+mV8XmA@mail.gmail.com>
 <CAD21AoA6i2ui8FMZeuU_KxX4t-fM8G==zTW2Dp6-goujttrpew@mail.gmail.com>
 <CAB-JLwZpp=7c9_r0beWWJxRh2BS_2Vvth8UDv7H57DBeaqggVg@mail.gmail.com>
 <CAD21AoDT3sL2COprsRumM9zEpL1Bk5VWboK4V2mRnjGua8xfeA@mail.gmail.com>
 <CAD21AoDQM62GOtaTzD_CVMSsFhv6o9c0Au1dSM1QuxeKFkWAKw@mail.gmail.com>
 <CAD21AoCz7HjEr3oeb=haK31YHxHZLcvD_wx_a-+xLPKywq++3A@mail.gmail.com>
 <TY4PR01MB16907733B75A99117F013AFCA947FA@TY4PR01MB16907.jpnprd01.prod.outlook.com>
 <CAD21AoA9YgiY1rVKMPZwB00WU_G4UfzoawY=7hyd7hpvBPcK6w@mail.gmail.com>
 <CAA4eK1KoSi60dtakJzn0MxNnHF1Yf4indSAffTjJxQG_31jsgQ@mail.gmail.com>
 <CAD21AoB4B3MOxJ7-v9YLjV5fTOtaLRUhX3jN3kqhEi7D7-uY4A@mail.gmail.com>
 <0c28fbd1-3320-4e9b-815c-6d62753aa063@wi3ck.info>
 <CAD21AoCmHpKrNg9D3mcOA973CZ5N_dBLxb8pERpSxEeRLSQxpA@mail.gmail.com>
 <CAD21AoAEVyxwn_bMWHvcU-Gcz3aUVjAtMbdgfoJ8MZNiLLEh0g@mail.gmail.com>
 <CAA4eK1Jkouj=w+PHzMB6v890ES3QOLf=cUTvZmGFr-WMQW2OnA@mail.gmail.com>
 <CAD21AoB4_n7+s=uM9apX1JVtvGvgM8ismAx_uMxvDmUXfQULsw@mail.gmail.com>
 <CAD21AoBJcxRcaWQot302diaxoDcsnezRhnZa7p8UrPh5AGNeHQ@mail.gmail.com>
 <CAHut+PuSkabUB8H_hcwQz=BX5TWEj-8Ba+CP_PX78zN1fkhtKA@mail.gmail.com>
 <CAA4eK1K+rumWz=mHDLVVCig-i_cWWSzwDE1eMySq0WYc7_ve+Q@mail.gmail.com>
 <TY4PR01MB169070A34D1C74867EF5A2DE49453A@TY4PR01MB16907.jpnprd01.prod.outlook.com>
 <CAD21AoBB3ovY4Hpr+HM6VkCY+sj3_71ZRi3oOP67w5ivvy4zuQ@mail.gmail.com>
 <CAD21AoCNvrrkqZFRofPdyW7sd0CoRsnZZc4RusFbLrZax0ELzw@mail.gmail.com>
 <CAHut+Pue4v3hVjCF0FNVHVn4-ZXdm05AXoU3RJJpYXjn2fs_jw@mail.gmail.com>
In-Reply-To: 
 <CAHut+Pue4v3hVjCF0FNVHVn4-ZXdm05AXoU3RJJpYXjn2fs_jw@mail.gmail.com>
From: Masahiko Sawada <sawada.mshk@gmail.com>
Date: Thu, 2 Apr 2026 15:13:18 -0700
Message-ID: 
 <CAD21AoAj-D9kFuxBTtiXjMNibJdpfeTe-FkN00bnuUG6mXoqVg@mail.gmail.com>
Subject: Re: Initial COPY of Logical Replication is too slow
To: Peter Smith <smithpb2250@gmail.com>
Cc: "Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>,
 Amit Kapila <amit.kapila16@gmail.com>,
	Jan Wieck <jan@wi3ck.info>,
	"pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAD21AoAj-D9kFuxBTtiXjMNibJdpfeTe-FkN00bnuUG6mXoqVg%40mail.gmail.com>
Precedence: bulk

On Wed, Apr 1, 2026 at 11:12=E2=80=AFPM Peter Smith <smithpb2250@gmail.com>=
 wrote:
>
> Hi Sawada-San
>
> Some review comments for v8-0001.
>
> =3D=3D=3D=3D=3D=3D
> Commit message
>
> 1.
> The existing a VARIADIC array form of pg_get_publication_tables() is
> preserved for backward compatibility. Tablesync workers use the new
> two-argument form when connected to a publisher running PostgreSQL 19
> or later.
>
> ~
>
> Typo? "The existing a VARIADIC"
>
> =3D=3D=3D=3D=3D=3D
> src/backend/catalog/pg_publication.c
>
> is_publishable_table:
>
> 2.
> + /*
> + * Sequences are publishable according to is_publishable_class() so
> + * explicitly exclude here.
> + */
> + if (relform->relkind !=3D RELKIND_SEQUENCE &&
> + is_publishable_class(tableoid, relform))
> + {
> + ReleaseSysCache(tuple);
> + return true;
> + }
>
> It seemed strange to say that "sequences are publishable according to
> is_publishable_class() so explicitly exclude", but then you proceed to
> call is_publishable_class() anyway.
>
> Maybe using a variable, and a different comment could be a better way
> of expressing this?
>
> SUGGESTION
> bool ret;
> ...
> /* Sequences are not tables, so this function returns false. */
> if (relform->relkind =3D=3D RELKIND_SEQUENCE)
>     ret =3D false;
> else
>     ret =3D is_publishable_class(tableoid, relform);
>
> ReleaseSysCache(tuple);
> return ret;
>
> ~~~
>
> is_table_publishable_in_publication:
>
> 3.
> + * A helper function for pg_get_publication_tables() to check whether th=
e
> + * table of the given relid is published for the specified publication.
>
> /table of the given relid/table with the given relid/
>
> /is published for the/is published in the/
>
> ~~~
>
> pg_get_publication_tables:
>
> 4.
> + * If filter_by_relid is true, only the row for target_relid is returned=
;
> + * if target_relid does not exist or is not part of the publications, ze=
ro
> + * rows are returned.  If filter_by_relid is false, rows for all tables
> + * within the specified publications are returned and target_relid is
> + * ignored.
>
> Should that say "only the row(s) for target_relid", e.g. possibly
> plural, because if same table is in multiple publications then there
> are be multiple result rows, right?
>
> =3D=3D=3D=3D=3D=3D
> src/include/catalog/pg_proc.dat
>
> 5.
> Missed my previous [1] review comment #4?
>
> First arg of pg_get_publication_tables_a should be plural 'pubnames',
> same as first arg of pg_get_publication_tables_b.
>
> =3D=3D=3D=3D=3D=3D
> src/test/regress/sql/publication.sql
>
> 6.
> +CREATE PUBLICATION pub_all_except_no_viaroot FOR ALL TABLES EXCEPT
> (TABLE tbl_parent, gpt_test_sch.tbl_sch) WITH
> (publish_via_partition_root =3D true);
>
> Why is this publication called '...no_viaroot' when
> publish_via_partition_root =3D true?
>
> ~~~
>
> 7.
> +-- test for the EXCLUDE clause
>
> Typo? /EXCLUDE clause/EXCEPT clause/
>

Thank you for the comments!

I've pushed the patch after incorporating these points.

Regards,

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