Re: Support EXCEPT for TABLES IN SCHEMA publications

Nisha,
A few comments on v24-001:

1)
+ if (is_except)
+ ereport(ERROR,
+ (errcode(ERRCODE_DUPLICATE_OBJECT),
+ errmsg("cannot add table \"%s\" to publication \"%s\"",
+ RelationGetQualifiedRelationName(targetrel),
+ pub->name),
+ errdetail("The table is named in the publication's EXCEPT clause for
schema \"%s\".",
+ get_namespace_name(RelationGetNamespace(targetrel))),
+ errhint("Change the EXCEPT clause using ALTER PUBLICATION ... SET
TABLES IN SCHEMA ... EXCEPT.")));
+ else
+ ereport(ERROR,
+ (errcode(ERRCODE_DUPLICATE_OBJECT),
+ errmsg("relation \"%s\" is already member of publication \"%s\"",
+ RelationGetRelationName(targetrel), pub->name)));

Does ERRCODE_INVALID_PARAMETER_VALUE suits better in 'if-block' rather
than ERRCODE_DUPLICATE_OBJECT?
See similar error you added in CheckExceptNotInTableList():

+ if (exceptrelid == explicitrelid)
+ ereport(ERROR,
+ errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("table \"%s\" cannot be both published and excluded",
+ RelationGetQualifiedRelationName(pri->relation)));

2)
+static void
+ProcessSchemaExceptTables(Oid schemaid, List *except_tables,

Since we have defined most of the arguments in comment section, we can
add 'pstate' as well for the sake of completion.

3)
Atop CheckExceptNotInTableList(), please add a comment indicating why
this funciton is still needed when we already have similar checks in
publication_add_relation and check_publication_add_relation.

4)
I was debugging the flow to see why 'CheckExceptNotInTableList' is
still needed when we have similar check in publication_add_relation().
My analysis:

publication_add_relation() accepts 'if_not_exists' i.e. add the new
entry 'if already not present' else skip it (no error if it is a
duplicate addition). Most flows pass it as 'true'. The current code
skips raising error if if_not_exists=true and entry exists. It made
sense earlier, but in our implementation, I feel it should still raise
an error if entries are cross wired (i.e., if an exclusion is present
and we are trying to add it as an inclusion, or vice versa). The
'if_not_exists' based 'skip logic' should only be exercised if the
nature of existing entity is of same kind as user is trying to add.
Let me know if you have different understanding.

Based on this, I tried debugging:

create publication pub1 for table s1.t1, tables in schema s1 except
(table s1.t1);

What changes I did:
--I skipped CheckExceptNotInTableList() invoked from
CreatePublication() so that flow directly hits the immediate next call
PublicationAddTables for schmea's except entries.
--In publication_add_relation(), I made this 'if_not_exists'
correction (by simply making it false for debuggin purpose).

The flow was as follows:

--CREATE-SUb first added table s1.t1 with prexcept=false through
PublicationAddTables.

--It then added Schmea to pg_pub_namespace using PublicationAddSchemas
and then invoked PublicationAddTables on except-table for the schema.
This step (against my expectation) could not find the entry added in
previous step (prexcept=false once) using CheckPublicationRelEntry()
and thus could not hit the required error. Instead it hit more
internal error:
ERROR: duplicate key value violates unique constraint
"pg_publication_rel_prrelid_prpubid_index"

So it seems the catalog change from the first step was not visible in
the second step. What am I missing? The rel-cache invalidation or
something else?

Nisha, can you debug and analyse in this direction? Expectation is to
either get rid of CheckExceptNotInTableList() or to conclude that
CheckExceptNotInTableList() is a reasonable addition to the code.

thanks
Shveta