MIME-Version: 1.0
References: 
 <CAJpy0uB20MhJJEaPJdm31t4fykJ+fChA_76jU2P9HX5knbJvAA@mail.gmail.com>
 <CAD21AoCC8XuwfX62qKBSfHUAoww_XB3_84HjswgL9jxQy696yw@mail.gmail.com>
 <OS9PR01MB12149EA0C749BC29C7C949E32F544A@OS9PR01MB12149.jpnprd01.prod.outlook.com>
 <CAD21AoBbZEshyaK0PeiF_J4_S75EfF=Gcs=C+X-osoVoUnawuQ@mail.gmail.com>
 <CAHut+PssG+sHeV+Xo0g=S7xBb9FgDPjHYDR4iSuOdYXDq-Psng@mail.gmail.com>
 <CAA4eK1LaSfAG7UAuy1xpnkWKM_YtrPuhbgAxYBFY3Sp_v_KqoQ@mail.gmail.com>
 <CAD21AoAb8E8krN63cY_U7RQs9v-zkqUZyKT_UVKDwKfExtvTBg@mail.gmail.com>
 <CAA4eK1K1GLR7DXSABayQE+pWM=v1ODD6haPYxuDhAYwJN5gjzg@mail.gmail.com>
 <CALDaNm2kvFahDDvdgCNo=Nv-COz_N5Xw8YmzQBN2bd3g=N81fQ@mail.gmail.com>
 <CAHut+PsCqTR_kQu5M1TqBjnE6KM5cO22aH8boHfpMa_gSJBmWg@mail.gmail.com>
 <CALDaNm2OOgmNOPpABUU+AXzHhfrLG9HMfSd3jfNe=t3dc-kp1Q@mail.gmail.com>
In-Reply-To: 
 <CALDaNm2OOgmNOPpABUU+AXzHhfrLG9HMfSd3jfNe=t3dc-kp1Q@mail.gmail.com>
From: shveta malik <shveta.malik@gmail.com>
Date: Tue, 17 Mar 2026 16:31:43 +0530
Message-ID: 
 <CAJpy0uCN4gfP7fSt__KdW5wYQ82650Z6L4YLnjRHZTQ1yir1mg@mail.gmail.com>
Subject: Re: Skipping schema changes in publication
To: vignesh C <vignesh21@gmail.com>
Cc: Peter Smith <smithpb2250@gmail.com>,
 Amit Kapila <amit.kapila16@gmail.com>,
	Masahiko Sawada <sawada.mshk@gmail.com>,
	"Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>,
 Shlok Kyal <shlok.kyal.oss@gmail.com>,
	Nisha Moond <nisha.moond412@gmail.com>,
 Ashutosh Sharma <ashu.coek88@gmail.com>,
	"David G. Johnston" <david.g.johnston@gmail.com>,
 Dilip Kumar <dilipbalaut@gmail.com>,
	"Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>, YeXiu <1518981153@qq.com>,
	Ian Lawrence Barwick <barwick@gmail.com>,
	Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
 shveta malik <shveta.malik@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAJpy0uCN4gfP7fSt__KdW5wYQ82650Z6L4YLnjRHZTQ1yir1mg%40mail.gmail.com>
Precedence: bulk

On Tue, Mar 17, 2026 at 12:23=E2=80=AFPM vignesh C <vignesh21@gmail.com> wr=
ote:
>
> 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</lite=
ral>
+   publication.  If <literal>EXCEPT TABLE</literal> is specified with a li=
st of
+   tables, the existing except table list is replaced with the
specified tables.
+   If <literal>EXCEPT TABLE</literal> is omitted, the existing except tabl=
e
+   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 =3D 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=3Dtrue 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 =3D (Form_pg_publication_rel) GETSTRUCT(tup);
   result =3D !pubrel->prexcept
}


4)
publication_add_relation:
+ /*
+ * True if EXCEPT tables require explicit relcache invalidation. If
+ * 'puballtables' changes, global invalidation covers them.
+ */
+ inval_except_table =3D (stmt !=3D NULL) &&
+ (stmt->for_all_tables =3D=3D 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