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

On Wed, Sep 1, 2021 at 8:52 AM Greg Nancarrow <gregn4422@gmail.com> wrote:
>
> On Tue, Aug 31, 2021 at 8:57 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > I find the way it is implemented to be more intuitive as that gives
> > users more flexibility to retain certain tables from the schema and
> > appears to be exactly what users intended by the command. I don't
> > think finding duplicates among different object lists (schema, table)
> > is a good idea because tomorrow for some other objects the same thing
> > can happen. It might be better to get some other opinions on this
> > matter though.
> >
>
> I think that such functionality needs to be clearly documented (but
> currently the documentation doesn't sufficiently explain it).
>

I agree that we should document it clearly if we decide to go in this direction.

> Yes, I would definitely like to hear other opinions on this.
>
> Note also that currently parts of the documentation are still
> referring to "ADD SCHEMA/DROP SCHEMA/SET SCHEMA" instead of the new
> syntax "ADD ALL TABLES IN SCHEMA/DROP ALL TABLES IN SCHEMA/SET ALL
> TABLES IN SCHEMA":
>
> e.g.
> v23-0003:
> doc/src/sgml/ref/alter_publication.sgml
> +   The fourth, fifth and sixth variants of this command change which schemas
> +   are part of the publication.  The <literal>SET SCHEMA</literal> clause will
> +   replace the list of schemas in the publication with the specified one.
> +   The <literal>ADD SCHEMA</literal> and <literal>DROP SCHEMA</literal> clauses
> +   will add and remove one or more schemas from the publication.  Note that
> +   adding schemas to a publication that is already subscribed to will require
> +   a <literal>ALTER SUBSCRIPTION ... REFRESH PUBLICATION</literal> action on
> +   the subscribing side in order to become effective.
>
> With the new syntax changes (SCHEMA -> ALL TABLES IN SCHEMA), it seems
> less intuitive that there are separate schema and table operations on
> the publication.
>

I think the documentation could be improved w.r.t above points because
it is quite clear that we support schema and tables separately.


> I'd expect a lot of users to naturally think that "ALTER PUBLICATION
> pub1 DROP ALL TABLES IN SCHEMA sc1;" would drop from the publication
> all tables that are in schema "sc1", which is not what it is currently
> doing.
> Since the syntax was changed to specifically refer to FOR ALL TABLES
> IN SCHEMA rather than FOR SCHEMA, then now it's clear we're referring
> to tables only, when specifying "... FOR ALL TABLES in sc1, TABLE
> sc1.test", so IMHO it's reasonable to remove duplicates here, rather
> than treating these as somehow separate ways of referencing the same
> table.
>

I see your point and if we decide to go this path then it is better to
give an error than silently removing duplicates.

> One thing the current scheme doesn't allow is to publish all tables in
> a schema except for certain table(s)
>

True, we can always support that as a separate feature. Note that same
is true for existing FOR ALL TABLES syntax where users could expect to
leave few tables with syntax like FOR ALL TABLES EXCEPT t1,t2,... but
we don't have that yet.

> - and you can't achieve that by
> adding all tables from a schema to the publication and then
> selectively dropping some of those tables. I thought that this would
> be a more common pattern than adding separate tables from schemas that
> are already included as part of the publication, in order to "retain"
> them if "all tables from schema ..." are later dropped.
>
> postgres=# create schema sc1;
> CREATE SCHEMA
> postgres=# create table sc1.test(i int);
> CREATE TABLE
> postgres=# create publication pub1 for all tables in schema sc1;
> CREATE PUBLICATION
> postgres=# \dRp+
>
>                             Publication pub1
>  Owner | All tables | Inserts | Updates | Deletes | Truncates | Via root
> -------+------------+---------+---------+---------+-----------+----------
>  gregn | f          | t       | t       | t       | t         | f
> Schemas:
>     "sc1"
>
> postgres=# alter publication pub1 drop table sc1.test;
> ERROR:  relation "test" is not part of the publication
>
> The above error message seems slightly misleading (as that table IS
> published by that publication) and also note the relation is not
> schema-qualified.
>

I think we should try to improve this message, maybe we can give
detail message similar to what we give For ALL Tables case (DETAIL:
Tables that are part of SCHEMA cannot be dropped independently).

--
With Regards,
Amit Kapila.