On Fri, May 22, 2026 at 11:00 AM Peter Smith <[email protected]> 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
