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

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

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