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

On Tuesday, August 3, 2021 11:08 PM vignesh C <vignesh21@gmail.com> wrote:
> 
> Thanks for reporting this, this is fixed in the v18 patch attached.

Thanks for fixing it.

Few suggestions for V18:

1. 
+# Clean up the tables on both publisher and subscriber as we don't need them
+$node_publisher->safe_psql('postgres', "DROP SCHEMA sch1 cascade");
+$node_publisher->safe_psql('postgres', "DROP SCHEMA sch2 cascade");
+$node_publisher->safe_psql('postgres', "DROP SCHEMA sch3 cascade");
+$node_subscriber->safe_psql('postgres', "DROP SCHEMA sch1 cascade");
+$node_subscriber->safe_psql('postgres', "DROP SCHEMA sch2 cascade");
+$node_subscriber->safe_psql('postgres', "DROP SCHEMA sch3 cascade");

Should we change the comment to "Clean up the schemas ... ", instead of 'tables'?

2.
+$result = $node_subscriber->safe_psql('postgres',
+        "SELECT count(*) FROM sch1.tab3");

Spaces are used here(and some other places), but in most places we use a TAB, so
I think it's better to change it to a TAB.

3.
    List       *tables;            /* List of tables to add/drop */
     bool        for_all_tables; /* Special publication for all tables in db */
     DefElemAction tableAction;    /* What action to perform with the tables */
+    List       *schemas;        /* Optional list of schemas */
 } AlterPublicationStmt;

Should we change the comment here to 'List of schemas to add/drop', then it can
be consistent with the comment for 'tables'.

I also noticed that 'tableAction' variable is used when we add/drop/set schemas,
so maybe the variable name is not suitable any more.

Besides, the following comment is above these codes. Should we add some comments
for schema?

/* parameters used for ALTER PUBLICATION ... ADD/DROP TABLE */

And it says 'add/drop', do we need to add 'set'? (it's not related to this
patch, so I think I can add it in another thread[1] if needed, which is related
to comment improvement)

4.
I saw the existing tests about permissions in publication.sql, should we add
tests for schema publication? Like this:

diff --git a/src/test/regress/sql/publication.sql b/src/test/regress/sql/publication.sql
index 33dbdf7bed..c19337631e 100644
--- a/src/test/regress/sql/publication.sql
+++ b/src/test/regress/sql/publication.sql
@@ -160,16 +160,19 @@ GRANT CREATE ON DATABASE regression TO regress_publication_user2;
 SET ROLE regress_publication_user2;
 SET client_min_messages = 'ERROR';
 CREATE PUBLICATION testpub2;  -- ok
+CREATE PUBLICATION testpub3;  -- ok
 RESET client_min_messages;

 ALTER PUBLICATION testpub2 ADD TABLE testpub_tbl1;  -- fail
+ALTER PUBLICATION testpub3 ADD SCHEMA pub_test;  -- fail

 SET ROLE regress_publication_user;
 GRANT regress_publication_user TO regress_publication_user2;
 SET ROLE regress_publication_user2;
 ALTER PUBLICATION testpub2 ADD TABLE testpub_tbl1;  -- ok
+ALTER PUBLICATION testpub3 ADD SCHEMA pub_test;  -- ok

-DROP PUBLICATION testpub2;
+DROP PUBLICATION testpub2, testpub3;

 SET ROLE regress_publication_user;
 REVOKE CREATE ON DATABASE regression FROM regress_publication_user2;


[1]
https://www.postgresql.org/message-id/flat/OS0PR01MB6113480F937572BF1216DD61FBEF9%40OS0PR01MB6113.jpnprd01.prod.outlook.com

Regards
Tang