Re: Skipping schema changes in publication

On Wed, 18 Mar 2026 at 12:11, Peter Smith <smithpb2250@gmail.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.

The comments from [2] have also been addressed in the attached patch.
[1] - https://www.postgresql.org/message-id/CAA4eK1LCh7iCy2raNRrfVaS0LJaHxYokCUaNVue-qLfR%3DiG1PA%40mail.gmail.com
[2] - https://www.postgresql.org/message-id/CAJpy0uCMnF25JNgx1vnQMV3FQoiiqdOJYwnCqYcOZ-g6B8%2BfWA%40mail.gmail.com

Regards,
Vignesh