RE: Added schema level support for publication.

On Tuesday, August 3, 2021 11:08 PM vignesh C <vignesh21(at)gmail(dot)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