Re: Added schema level support for publication. - Mailing list pgsql-hackers

On Thu, Oct 7, 2021 at 5:19 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Oct 6, 2021 at 11:12 AM vignesh C <vignesh21@gmail.com> wrote:
> >
> > Attached v37 patch has the changes for the same.
> >
>
> Few comments:
> ==============
>

Few more comments:
====================
v37-0002-Client-side-changes-to-support-FOR-ALL-TABLES-IN
1.
+ else if (Matches("CREATE", "PUBLICATION", MatchAny, "FOR", "ALL",
"TABLES", "IN", "SCHEMA"))
+ COMPLETE_WITH_QUERY(Query_for_list_of_schemas
+ " UNION SELECT 'CURRENT_SCHEMA' "
+ "UNION SELECT 'WITH ('");

What is the need to display WITH here? It will be displayed after
Schema name with the below rule:
+ else if (Matches("CREATE", "PUBLICATION", MatchAny,  "FOR", "ALL",
"TABLES", "IN", "SCHEMA", MatchAny))
+ COMPLETE_WITH("WITH (");

2. It seems tab-completion happens incorrectly for the below case:
create publication pub for all tables in schema s1,

If I press the tab after above, it completes with below which is wrong
because it will lead to incorrect syntax.

create publication pub for all tables in schema s1, WITH (

v37-0003-Tests-for-FOR-ALL-TABLES-IN-SCHEMA-publication
3.
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
..
+ 'ALTER PUBLICATION pub3 ADD ALL TABLES IN SCHEMA dump_test' => {
+ create_order => 51,
+ create_sql =>
+   'ALTER PUBLICATION pub3 ADD ALL TABLES IN SCHEMA dump_test;',
+ regexp => qr/^
+ \QALTER PUBLICATION pub3 ADD ALL TABLES IN SCHEMA dump_test;\E
+ /xm,
+ like   => { %full_runs, section_post_data => 1, },
+ unlike => { exclude_dump_test_schema => 1, },

In this test, won't it exclude the schema dump_test because of unlike?
If so, then we don't have coverage for "ALL Tables In Schema" except
for public schema?

4.
--- a/src/test/regress/expected/publication.out
+++ b/src/test/regress/expected/publication.out
..
+-- fail - can't add schema to for all tables publication
+ALTER PUBLICATION testpub_foralltables ADD ALL TABLES IN SCHEMA pub_test;

In the above and all similar comments, it is better to either quote
'for all tables' or write in CAPS FOR ALL TABLE or both 'FOR ALL
TABLE'.

5.
+\dRp+ testpub1_forschema
+                               Publication testpub1_forschema
+          Owner           | All tables | Inserts | Updates | Deletes
| Truncates | Via root
+--------------------------+------------+---------+---------+---------+-----------+----------
+ regress_publication_user | f          | t       | t       | t
| t         | f
+Tables from schemas:
+    "pub_test1"
+
+SELECT p.pubname FROM pg_catalog.pg_publication p,
pg_catalog.pg_namespace n, pg_catalog.pg_publication_namespace pn
WHERE n.oid = pn.pnnspid AND p.oid = pn.pnpubid AND n.nspname =
'pub_test1' ORDER BY 1;
+      pubname
+--------------------
+ testpub1_forschema
+(1 row)

I don't think in the above and similar tests, we need to separately
check the presence of publication via Select query, if we have tested
it via psql command. Let's try to keep the meaningful tests.

6.
+INSERT INTO pub_test1.tbl VALUES(1, 'test');
+-- fail
+UPDATE pub_test1.tbl SET id = 2;
+ERROR:  cannot update table "tbl" because it does not have a replica
identity and publishes updates
+HINT:  To enable updating the table, set REPLICA IDENTITY using ALTER TABLE.
+ALTER PUBLICATION testpub1_forschema DROP ALL TABLES IN SCHEMA pub_test1;
+-- success
+UPDATE pub_test1.tbl SET id = 2;
+ALTER PUBLICATION testpub1_forschema SET ALL TABLES IN SCHEMA pub_test1;
+-- fail
+UPDATE pub_test1.tbl SET id = 2;
+ERROR:  cannot update table "tbl" because it does not have a replica
identity and publishes updates
+HINT:  To enable updating the table, set REPLICA IDENTITY using ALTER TABLE.
+ALTER PUBLICATION testpub1_forschema DROP ALL TABLES IN SCHEMA pub_test1;
+-- success
+UPDATE pub_test1.tbl SET id = 2;
+ALTER PUBLICATION testpub1_forschema ADD ALL TABLES IN SCHEMA pub_test1;
+-- fail
+UPDATE pub_test1.tbl SET id = 2;
+ERROR:  cannot update table "tbl" because it does not have a replica
identity and publishes updates
+HINT:  To enable updating the table, set REPLICA IDENTITY using ALTER TABLE.

I think here we don't need to test both SET and ADD variants, one of
them is sufficient.

7.
+-- verify invalidation of partition table having partition on different schema

I think this comment is not very clear to me. Can we change it to:
"verify invalidation of partition table having parent and child tables
in different schema"?

8.
+-- verify invalidation of schema having partition parent table and
partition child table

Similarly, let's change this to: "verify invalidation of partition
tables for schema publication that has parent and child tables of
different partition hierarchies". Keep comments line boundary as 80
chars, that way they look readable.

9.
+++ b/src/test/subscription/t/025_rep_changes_for_schema.pl
..
+# Basic logical replication test

Let's change this comment to: "Logical replication tests for schema
publications"


-- 
With Regards,
Amit Kapila.