Re: Support EXCEPT for TABLES IN SCHEMA publications

On Fri, May 22, 2026 at 11:00 AM Peter Smith <smithpb2250(at)gmail(dot)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="parameter">except_table_object</replaceable> [, ... ]
> )</literal></term>
> + <listitem>
> + <para>
> + Specifies tables to be excluded from a schema-level publication entry.
> + This clause may be used with <literal>ADD TABLES IN SCHEMA</literal>
> + 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="sql-createpublication"/> for further details on the
> + 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.

> ======
> 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 == 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 == 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 = list_concat(relations, exceptrelations);
> AlterPublicationTables(stmt, tup, relations, pstate->p_sourcetext,
> schemaidlist != 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