Re: Initial COPY of Logical Replication is too slow

Hi Sawada-San

Some review comments for v8-0001.

======
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"

======
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 != 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 == RELKIND_SEQUENCE)
ret = false;
else
ret = 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 the
+ * 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, zero
+ * 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?

======
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.

======
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 = true);

Why is this publication called '...no_viaroot' when
publish_via_partition_root = true?

~~~

7.
+-- test for the EXCLUDE clause

Typo? /EXCLUDE clause/EXCEPT clause/

======
[1] https://www.postgresql.org/message-id/CAHut%2BPuSkabUB8H_hcwQz%3DBX5TWEj-8Ba%2BCP_PX78zN1fkhtKA%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia