Re: Skipping schema changes in publication

On Tue, Mar 17, 2026 at 12:23 PM vignesh C <vignesh21@gmail.com> wrote:
>
> Thanks for the comments, the agreed comments have been addressed in
> the v64 version patch attached.
>

Please find a few comments:


1)
+ The
+   <literal>SET ALL TABLES</literal> clause is used to update the
+   <literal>EXCEPT TABLE</literal> list of a <literal>FOR ALL TABLES</literal>
+   publication.  If <literal>EXCEPT TABLE</literal> is specified with a list of
+   tables, the existing except table list is replaced with the
specified tables.
+   If <literal>EXCEPT TABLE</literal> is omitted, the existing except table
+   list is cleared.

How about changing it to (or anything better to reflect new changes):

The SET ALL TABLES clause can transform an empty publication, or one
defined for ALL SEQUENCES (or both ALL TABLES and ALL SEQUENCES), into
a publication defined for ALL TABLES. Likewise, SET ALL SEQUENCES can
convert an empty publication, or one defined for ALL TABLES (or both
ALL TABLES and ALL SEQUENCES), into a publication defined for ALL
SEQUENCES.
In addition, SET ALL TABLES may be used to update the EXCEPT TABLE
list of a FOR ALL TABLES publication. If EXCEPT TABLE is specified
with a list of tables, the existing exclusion list is replaced with
the specified tables. If EXCEPT TABLE is omitted, the existing
exclusion list is cleared.

2)
+bool
+is_include_relation_publication(Oid pubid)

The name 'is_include_relation_publication' looks slightly odd to me.
Few options are: is_explicit_table_publication,
is_table_list_publication, is_table_publication. Or anything better if
you can think of?

3)
is_include_relation_publication:

+ /* If we find even one included relation, we are done */
+ if (!pubrel->prexcept)
+ {
+ result = true;
+ break;
+ }

we can break the loop irrespective of the 'prexcept' flag as we can
never have a combination of mixed prexcept entries for the same pub.
Whether all will be with prexcept=true or all will be false. Even a
loop is not needed. We can fetch the first entry alone (similar to
is_schema_publication) and if that is valid, we can check the flag and
return accordingly. Something like:

if (HeapTupleIsValid())
{
   pubrel = (Form_pg_publication_rel) GETSTRUCT(tup);
   result = !pubrel->prexcept
}


4)
publication_add_relation:
+ /*
+ * True if EXCEPT tables require explicit relcache invalidation. If
+ * 'puballtables' changes, global invalidation covers them.
+ */
+ inval_except_table = (stmt != NULL) &&
+ (stmt->for_all_tables == pub->alltables);


It took me some time to figure out why we don't need invalidation for
the case where we are converting ALL SEQ to ALL TABLEs EXCEPT(..).  I
think it is worth adding more comments here. Suggestion:

/*
 * 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.
 */

thanks
Shveta