On Wed, Mar 18, 2026 at 12:11 PM Peter Smith <[email protected]> wrote:
>
> ======
> doc/src/sgml/ref/alter_publication.sgml
>
> 1.
> It seems that SET ALL TABLES is not supported if the publication
> already has FOR TABLE.
>
> e.g
> alter publication pub1 set all tables;
> ERROR: publication "pub1" does not support ALL TABLES operations
> DETAIL: This operation requires the publication to be defined as FOR
> ALL TABLES/SEQUENCES or to be empty.
>
> e.g.
> alter publication pub2 set table t1;
> ERROR: publication "pub2" is defined as FOR ALL TABLES
> DETAIL: Tables or sequences cannot be added to or dropped from FOR
> ALL TABLES publications.
>
> I am not going to debate what rules are right or wrong. (Some rules do
> seem a bit ad-hoc to me, but maybe they are just erring on the safety
> side). But my point is that the documentation does not seem to say
> anything much about what the rules are
>
> e.g. it says "Adding/Setting any schema when the publication also
> publishes a table with a column list, and vice versa is not
> supported.", but OTOH it says nothing about what is
> supported/unsupported for SET ALL TABLES.
>
Most of these rules pre-exist this patch, we can add more docs for
these but not sure if it is worth it.
> ======
> src/backend/catalog/pg_publication.c
>
> 2.
> +/*
> + * Returns true if the publication has explicitly included relation (i.e.,
> + * not marked as EXCEPT).
> + */
> +bool
> +is_table_publication(Oid pubid)
>
> To me, an "explicitly included relation" is like when you say "FOR
> TABLE t1", where the "t1" is explicitly named.
>
> So it's not very clear whether you consider "FOR ALL TABLES" or a
> "FOR TABLES IN SCHEMA" publication explicitly includes tables or not?
>
> The function comment needs to be clearer.
>
It is clear for me that it means ("FOR TABLE t1") and follows what we
wrote atop is_schema_publication.
>
> AlterPublication:
>
> 6.
> + relations = list_concat(relations, exceptrelations);
> AlterPublicationTables(stmt, tup, relations, pstate->p_sourcetext,
> schemaidlist != NIL);
>
> I did not quite understand this list_concat.
>
> Is this somehow asserting that `relations` must be empty when there
> were `exceptrelations` and vice versa, because other combinations are
> not supported by ALTER -- e.g. is this just a trick to so you can pass
> the same parameter to AlterPublicationTables? This seems related to
> the 'isexecpt' AlterPublicationTables function, which was also quite
> subtle.
>
> Bottom line is, I'm unsure what the logic is here, but it appears
> overly tricky to me. Would more comments help?
>
The logic is quite simple here, we need to treat both types of
included/excluded relations the same w.r.t recording them in catalog.
The only difference is for excluded relations, except flag will be
true. I feel adding comments for such simple stuff is not required,
otherwise, the code will be filled with comments explaining simple
things which one can easily make out if she understands the logic
behind the code.
--
With Regards,
Amit Kapila.
