Re: Support EXCEPT for TABLES IN SCHEMA publications

On Fri, May 15, 2026 at 12:49 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Hi Nisha.
>
> Some review comments for patch v5-0001.
>

Thanks Peter, for the review.

> ======
> src/backend/catalog/pg_publication.c
>
> publication_add_relation:
>
> 2.
> + HeapTuple existing;
>
> Not sure if this is the best name. How about "tup"?
>

Noticed that a "HeapTuple tup;" is already declared, so we can use the
existing one.

> ~~~
>
> 3.
> + bool is_except = existing_form->prexcept;
>
> This variable is used only once. Not sure if that vindicates having it.
>

the is_except value is being used after releasing the tuple and
closing the table since the next step errors out. But I have now
removed existing_form and directly extracted the value instead.

> ~~~
>
> 8.
> + list_free(schemaRels);
> + }
> + else
> + result = list_concat(result, schemaRels);
>
> Why is 'schemaRels' only being freed when there is an EXCEPT?
>

IIUC, In the EXCEPT case, relid is an Oid, so lfirst_oid() copies the
integer value from the cell, and lappend_oid() stores that value into
a new cell in result. That means result does not reference schemaRels
cells after the loop, so list_free(schemaRels) is safe.

In the else branch, list_concat() directly transfers schemaRels cells
into result. So freeing schemaRels there would corrupt the result.

> ======
> src/backend/parser/gram.y
>
> 14.
> - | ColId opt_column_list OptWhereClause
> + | ColId opt_column_list OptWhereClause opt_pub_except_clause
> {
> $$ = makeNode(PublicationObjSpec);
> $$->pubobjtype = PUBLICATIONOBJ_CONTINUATION;
> + $$->except_tables = $4;
>
> This seems suspicious. You cannot have an EXCEPT clause when there is
> a column list or a WHERE clause, so what is this scenario? Maybe the
> "$$->except_tables = $4;" needs to be moved to the 'else' block?
>

This handles cases where multi-schema continuation is used along with
an EXCEPT clause, e.g.:
create publication pub1 for tables in schema public, s1 except (table t1);
-- without the above, the EXCEPT clause would be silently ignored.

Now, I agree that the above case can also be handled by moving
"$$->except_tables = $4;" into the else branch. But then EXCEPT would
again get silently ignored for table continuations with a column-list
or where clause, e.g.,:
postgres=# create publication pub2 for table t1, t2 (c1) except (table t1);
CREATE PUBLICATION

The idea here is to collect the EXCEPT list whenever it is specified
for such continuation cases, and then explicitly raise an error in
preprocess_pubobj_list() if a table publication object contains an
EXCEPT list.

> ~~~
>
> 16.
> + if (pubobj->pubobjtype == PUBLICATIONOBJ_TABLES_IN_SCHEMA)
> + {
> + if (eobj->pubtable->relation->schemaname == NULL)
> + eobj->pubtable->relation->schemaname = pubobj->name;
> + else if (strcmp(eobj->pubtable->relation->schemaname,
> + pubobj->name) != 0)
> + ereport(ERROR,
> + errcode(ERRCODE_SYNTAX_ERROR),
> + errmsg("table \"%s.%s\" in EXCEPT clause does not belong to schema \"%s\"",
> + eobj->pubtable->relation->schemaname,
> + eobj->pubtable->relation->relname,
> + pubobj->name),
> + parser_errposition(eobj->location));
>
> 16a.
> Introducing some more variables (like 'eobj_schemaname' and
> 'eobj_relname') would simplify this code quite a bit.
>

Done.

> ~
>
> 16b.
> Should make use of the recently committed function that gets
> fully-qualified rel names so you can use "%s" instead of "%s.%s".
>

We cannot use RelationGetQualifiedRelationName() in the grammar, so I
used quote_qualified_identifier() instead.
~~~~

Addressed all other comments as suggested. Please find the updated v6
patches attached.

Patch-0001: updated as per the above comments.
Patch-0002 and Patch-0003: adjusted for the Patch-0001 changes and
some code simplification in tab-complete part.

--
Thanks,
Nisha