Re: Skipping schema changes in publication

On Wed, 18 Mar 2026 at 18:51, vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> On Wed, 18 Mar 2026 at 12:11, Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> >
> > Hi Vignesh.
> >
> > Some review comments for patch v65-0001.
> >
> > ======
> > 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.
>
> Amit has already replied to this at [1].
>
> > ======
> > 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.
>
> Amit has already replied to this at [1].
>
> > ~~~
> >
> > publication_add_relation:
> >
> > 3.
> > + /*
> > + * Determine whether EXCEPT tables require explicit relcache invalidation.
> > + *
> > + * For CREATE PUBLICATION with EXCEPT tables, invalidation is not needed,
> > + * since it is handled when marking the publication as ALL TABLES.
> > + *
> > + * For ALTER PUBLICATION, invalidation is needed only when adding an EXCEPT
> > + * table to a publication already marked as ALL TABLES. For publications
> > + * that were originally empty or defined as ALL SEQUENCES and are being
> > + * converted to ALL TABLES, invalidation is skipped here, as it is handled
> > + * when marking the publication as ALL TABLES.
> > + */
> > + inval_except_table = (stmt != NULL) &&
> > + (stmt->for_all_tables == pub->alltables);
> >
> > 3a.
> > Why is this code done at the top of the function? Can you move it to
> > be adjacent to where it is getting used?
>
> Modified
>
> > ~
> >
> > 3b.
> > I think 'alter_stmt' or 'alter_pub_stmt' might be a more informative
> > name here, instead of the generic 'stmt'
>
> Modified
>
> > ======
> > src/backend/commands/publicationcmds.c
> >
> > 4.
> > - TransformPubWhereClauses(rels, queryString, pubform->pubviaroot);
> > + if (isexcept)
> > + oldrelids = GetExcludedPublicationTables(pubid,
> > + PUBLICATION_PART_ROOT);
> > + else
> > + {
> > + oldrelids = GetIncludedPublicationRelations(pubid,
> > + PUBLICATION_PART_ROOT);
> >
> > I felt there were some subtle things that the logic is using here:
> >
> > e.g.
> > It seems that because this function is called...
> > And because it is SET ....
> > And because it is SET ALL TABLES ...
> > Then, the tables can only be those in the EXCEPT TABLE list
> >
> > Maybe more comments about 'isexcept', and maybe an Assert(oldrelids !=
> > NIL); could help here (??)
>
> Updated comments
>
> > ~~~
> >
> > AlterPublicationAllFlags:
> >
> > 5.
> > + bool nulls[Natts_pg_publication];
> > + bool replaces[Natts_pg_publication];
> > + Datum values[Natts_pg_publication];
> > + bool dirty = false;
> > +
> > + if (!stmt->for_all_tables && !stmt->for_all_sequences)
> > + return;
> > +
> > + pubform = (Form_pg_publication) GETSTRUCT(tup);
> > +
> > + memset(values, 0, sizeof(values));
> > + memset(nulls, false, sizeof(nulls));
> > + memset(replaces, false, sizeof(replaces));
> >
> > AFAIK, these memsets are not needed if you just say "= {0}" where
> > those vars are declared.
>
> modified
>
> > 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?
>
> Amit has already replied to this at [1].
>
> > ======
> > src/backend/parser/gram.y
> >
> > 7.
> > + * ALL TABLES [ EXCEPT TABLE ( table_name [, ...] ) ]
> >
> > The CREATE/ALTER docs synopsis says "[ ONLY ] table_name [ * ]" which
> > is different from this comment. So are ONLY and * handled properly or
> > not?
>
> It is handled, Is there any issue with the code? It is documented that
> way to keep it consistent with CreatePublicationStmt and ALTER
> PUBLICATION ADD/DROP/SET pub_obj.
>
> > ======
> > src/include/nodes/parsenodes.h
> >
> > 8.
> > + bool for_all_tables; /* True if SET ALL TABLES is specified */
> > + bool for_all_sequences; /* True if SET ALL SEQUENCES is specified */
> >
> > Maybe these comments do not need to mention the keyword "SET".
> >
> > That way, in future if/when ADD/DROP get implemented, then this code
> > won't need to churn again.
>
> Modified
>
> The attached v66 version patch has the changes for the same.

While reviewing the patch I noticed couple of small improvements that
could be done:
1) changed "may be used " to "can be used" as that is the convention followed:
In addition,
+ <literal>SET ALL TABLES</literal> may be used to update the
+ <literal>EXCEPT TABLE</literal> list of a <literal>FOR ALL TABLES</literal>
+ publication.

2) This condition was slightly difficult to understand while reading,
simplified it:
Changed:
- if (!pri->except)
+ inval_except_table = (alter_stmt != NULL) &&
+ (alter_stmt->for_all_tables == pub->alltables);
to:
- if (!pri->except)
+ inval_except_table = (alter_stmt != NULL) && pub->alltables &&
+ (alter_stmt->for_all_tables && pri->except);

The attached v67 patch has the changes for the same.

Regards,
Vignesh