MIME-Version: 1.0
References: 
 <CABdArM5sw4Q1ZU8HGdo4BSc1A_+8xtUNq17j6wcir=yMUy19Cg@mail.gmail.com>
 <CAHut+PvnH8QHa035Skoh1e9jm_H08DO9fQ=F-NAMsEpYf0RZ2Q@mail.gmail.com>
 <CAJpy0uDu0LcNXcZCP0cR_LHqo+sau33KwPFHemmGVYf_JTxRBQ@mail.gmail.com>
 <CAA4eK1KbCWBmEXH-rhQjKgNwq=onZp8vRR-QkRhPpbKwL-kQdw@mail.gmail.com>
 <CAHut+Pvj4=GWoJEd4EBdp4pi6KxXQ46ioW=PV+=UktiXr2gCvg@mail.gmail.com>
 <CABdArM75F0A+DGP8AOt-_b_XREX40rvFid1jRjnr_+S5b51t8Q@mail.gmail.com>
 <CAJpy0uDTshb243L5yEYWB3uO-JrwSoRqQDNovh03K2GZuuR3Pg@mail.gmail.com>
 <CAJpy0uDy97ULmJUwPacAzc5u2seuPK6RXgCS1rnsW2MfR4eeSw@mail.gmail.com>
 <CABdArM6oXXXSAxxXFktTTfBf4kyxJCvdNtTbUZtSwJ=CepN+Xw@mail.gmail.com>
 <CAJpy0uBqM+fq7+g1ZRATuY16H10MFP9i25wfFCYCE5MGu+PE0Q@mail.gmail.com>
 <CABdArM4uKaS1coCQj6rAwMmHqU_cCJyEWNic-PFF1_ZjDDM82Q@mail.gmail.com>
 <CAHut+Pu5VNakf5JAhKM7T-P_q37eN1Qgv5nvZUe+8RAAT41y4g@mail.gmail.com>
 <CABdArM6WTm2gP4pcjVdHT1Nx6zdLKTq7nLPUkzZOhprc95a6Rw@mail.gmail.com>
 <CAHut+Ptthc1X-UA8-6zG-iFeCDuoNd+oJRBZ1eCnJ9RNOXjfBQ@mail.gmail.com>
 <CABdArM79m7-CTf6KGGGU2QBydFtuonGgfxRSqk-vhwTsH8z1ow@mail.gmail.com>
 <CAHut+Ps3sghX4qv1jehqMgvZG7DmUoAFqgjmVc5xUy+v5kHN3w@mail.gmail.com>
In-Reply-To: 
 <CAHut+Ps3sghX4qv1jehqMgvZG7DmUoAFqgjmVc5xUy+v5kHN3w@mail.gmail.com>
From: Nisha Moond <nisha.moond412@gmail.com>
Date: Thu, 28 May 2026 16:58:10 +0530
Message-ID: 
 <CABdArM5+Aj8WAHsh395dudyxgZxBUk-hasyEohDtASvdo0wBDw@mail.gmail.com>
Subject: Re: Support EXCEPT for TABLES IN SCHEMA publications
To: Peter Smith <smithpb2250@gmail.com>
Cc: shveta malik <shveta.malik@gmail.com>,
 Amit Kapila <amit.kapila16@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CABdArM5%2BAj8WAHsh395dudyxgZxBUk-hasyEohDtASvdo0wBDw%40mail.gmail.com>
Precedence: bulk

On Fri, May 22, 2026 at 11:00=E2=80=AFAM Peter Smith <smithpb2250@gmail.com=
> wrote:
>
> Hi Nisha.
>
> Here are some review comments for patch v6-0002.
>

Thanks for the review. All comments are addressed in v7. Please find
responses below for a few of the comments.

>
> EXCEPT
>
> 5.
> +   <varlistentry>
> +    <term><literal>EXCEPT ( <replaceable
> class=3D"parameter">except_table_object</replaceable> [, ... ]
> )</literal></term>
> +    <listitem>
> +     <para>
> +      Specifies tables to be excluded from a schema-level publication en=
try.
> +      This clause may be used with <literal>ADD TABLES IN SCHEMA</litera=
l>
> +      and not with <literal>DROP TABLES IN SCHEMA</literal>.  Each named
> +      table must belong to the schema specified in the same
> +      <literal>TABLES IN SCHEMA</literal> clause.  Table names may be
> +      schema-qualified or unqualified; unqualified names are implicitly
> +      qualified with the schema named in the same clause.  See
> +      <xref linkend=3D"sql-createpublication"/> for further details on t=
he
> +      semantics of <literal>EXCEPT</literal>.
> +     </para>
> +    </listitem>
> +   </varlistentry>
> +
>
> 5a.
> Oh! If this EXCEPT part was previously missing even for the "FOR ALL
> TABLES", then IMO that is a separate bug that should be in another
> thread and patched/fixed asap, then your patch should just make small
> changes to to it.
>

Agree. I'll make a separate patch for it and start a thread.

> =3D=3D=3D=3D=3D=3D
> src/backend/commands/publicationcmds.c
>
> AlterPublicationSchemas:
>
> 7.
> + /*
> + * Increment the command counter so that is_schema_publication() in
> + * GetExcludedPublicationTables() can see the just-inserted schema
> + * rows when AlterPublicationExceptTables runs next.
> + */
> + CommandCounterIncrement();
>
> Should this cut/paste common code be done afterwards just if
> stmt->action =3D=3D AP_AddObjects/SetObjects ?
>

Agree, fixed.

> ~~~
>
> AlterPublicationExceptTables:
>
> 8.
> + /*
> + * This function handles EXCEPT entries for schema-level publications
> + * only.  For FOR ALL TABLES publications, EXCEPT entries are already
> + * processed by AlterPublicationTables().
> + */
> + if (schemaidlist =3D=3D NIL && !is_schema_publication(pubid))
> + return;
>
> Huh? It seems contrary to the function comment that was also talking
> about "FOR ALL TABLES".
>

Corrected the function comments. It is for only schema publications.

> Should this function really be called something different like
> 'AlterPublicationSchemaExceptTables'?
>

Done. Renamed as suggested.

> ~~~
>
> 12.
> + /*
> + * For FOR ALL TABLES, EXCEPT entries are processed by
> + * AlterPublicationTables(), so merge them in.  For TABLES IN SCHEMA,
> + * they are handled separately by AlterPublicationExceptTables().
> + */
> + if (stmt->for_all_tables)
> + relations =3D list_concat(relations, exceptrelations);
>   AlterPublicationTables(stmt, tup, relations, pstate->p_sourcetext,
>      schemaidlist !=3D NIL);
>   AlterPublicationSchemas(stmt, tup, schemaidlist);
> + AlterPublicationExceptTables(stmt, tup, exceptrelations, schemaidlist);
>
> Would it be simpler if AlterPublicationExceptTables was merged (or
> delegated from) the AlterPublicationSchemas?
>

+1, fixed.

--
Thanks,
Nisha