Re: why can't a table be part of the same publication as its schema

On Mon, Sep 19, 2022 at 8:46 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> > diff --git a/doc/src/sgml/logical-replication.sgml b/doc/src/sgml/logical-replication.sgml
> > index 1ae3287..0ab768d 100644
> > --- a/doc/src/sgml/logical-replication.sgml
> > +++ b/doc/src/sgml/logical-replication.sgml
> > @@ -1120,6 +1120,11 @@ test_sub=# SELECT * FROM child ORDER BY a;
> >    </para>
> >
> >    <para>
> > +   Specifying a column list when the publication also publishes
> > +   <literal>FOR ALL TABLES IN SCHEMA</literal> is not supported.
> > +  </para>
> > +
> > +  <para>
> >     For partitioned tables, the publication parameter
> >     <literal>publish_via_partition_root</literal> determines which column list
> >     is used. If <literal>publish_via_partition_root</literal> is
> >
> > diff --git a/doc/src/sgml/ref/create_publication.sgml b/doc/src/sgml/ref/create_publication.sgml
> > index 0a68c4b..0ced7da 100644
> > --- a/doc/src/sgml/ref/create_publication.sgml
> > +++ b/doc/src/sgml/ref/create_publication.sgml
> > @@ -103,17 +103,17 @@ CREATE PUBLICATION <replaceable class="parameter">name</replaceable>
> >       </para>
> >
> >       <para>
> > +      Specifying a column list when the publication also publishes
> > +      <literal>FOR ALL TABLES IN SCHEMA</literal> is not supported.
> > +     </para>
> >
> > @@ -733,6 +694,24 @@ CheckPubRelationColumnList(List *tables, const char *queryString,
> >                       continue;
> >
> >               /*
> > +              * Disallow specifying column list if any schema is in the
> > +              * publication.
> > +              *
> > +              * XXX We could instead just forbid the case when the publication
> > +              * tries to publish the table with a column list and a schema for that
> > +              * table. However, if we do that then we need a restriction during
> > +              * ALTER TABLE ... SET SCHEMA to prevent such a case which doesn't
> > +              * seem to be a good idea.
> > +              */
> > +             if (publish_schema)
> > +                     ereport(ERROR,
> > +                                     errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> > +                                     errmsg("cannot use publication column list for relation \"%s.%s\"",
> > +                                                get_namespace_name(RelationGetNamespace(pri->relation)),
> > +                                                RelationGetRelationName(pri->relation)),
> > +                                     errdetail("Column list cannot be specified if any schema is part of the publication or specified in the list."));
> > +
>
> This seems a pretty arbitrary restriction.  It feels like you're adding
> this restriction precisely so that you don't have to write the code to
> reject the ALTER .. SET SCHEMA if an incompatible configuration is
> detected.  But we already have such checks in several cases, so I don't
> see why this one does not seem a good idea.
>

I agree that we have such checks at other places as well and one
somewhat similar is in ATPrepChangePersistence().

ATPrepChangePersistence()
{
...
...
/*
     * Check that the table is not part of any publication when changing to
     * UNLOGGED, as UNLOGGED tables can't be published.
     */

However, another angle to look at it is that we try to avoid adding
restrictions in other DDL commands for defined publications. I am not
sure but it appears to me Peter E. is not in favor of restrictions in
other DDLs. I think we don't have a strict rule in this regard, so we
are trying to see what makes the most sense based on feedback and do
it accordingly.

> The whole FOR ALL TABLES IN SCHEMA thing seems pretty weird in several
> aspects.  Others have already commented about the syntax, which is
> unlike what GRANT uses; I'm also surprised that we've gotten away with
> it being superuser-only.  Why are we building more superuser-only
> features in this day and age?  I think not even FOR ALL TABLES should
> require superuser.
>

The intention was to be in sync with FOR ALL TABLES.

-- 
With Regards,
Amit Kapila.