On Wed, 18 Mar 2026 at 12:11, Peter Smith <[email protected]> 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
v66-0001-Add-support-for-EXCEPT-TABLE-in-ALTER-PUBLICATIO.patch
Description: Binary data
