Re: Added schema level support for publication.

On Wed, Aug 4, 2021 at 8:08 PM tanghy(dot)fnst(at)fujitsu(dot)com <
tanghy(dot)fnst(at)fujitsu(dot)com> wrote:
>
> 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'?

Modified.

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

Modified.

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

Modified.

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

Changed the variable name.

> Besides, the following comment is above these codes. Should we add some
comments
> for schema?
>
> /* parameters used for ALTER PUBLICATION ... ADD/DROP TABLE */

Modified

> 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)

You can include the change in the patch posted.

> 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;

Added.

The changes for the above are available in the v19 patch posted at [1].

[1] -
https://www.postgresql.org/message-id/CALDaNm3BMLBpWOSdS3Q2vwpsM%3D0yovpJm8dKHRqNyFpANbrhpw%40mail.gmail.com

Regards,
Vignesh