On Tue, Mar 17, 2026 at 12:23 PM vignesh C <[email protected]> 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
