Re: Support EXCEPT for TABLES IN SCHEMA publications

Hi Nisha.

Here are some review comments for patch v6-0001.

======
src/backend/catalog/pg_publication.c

GetTopMostAncestorInPublication:

1.
+GetTopMostAncestorInPublication(Oid puboid, List *ancestors,
+ int *ancestor_level, List *except_pubids)

I am having dificulty understanding this function. There needs to be a
description what does the input parameter 'except_pubids' mean. The
param name doesn't tell me anything much -- just that it is a list of
pubids that "something" (what?) is excluded from. And how does that
relate to the 'ancestors'?

~~~

2.
{
aschemaPubids = GetSchemaPublications(get_rel_namespace(ancestor));
- if (list_member_oid(aschemaPubids, puboid))
+ if (list_member_oid(aschemaPubids, puboid) &&
+ !list_member_oid(except_pubids, puboid))

Is this new code in the right place? I'm not 100% sure of the
'except_pubids' details, but shouldn't it be checked sooner? e.g. if
we know already that this pubid is no good
(!list_member_oid(except_pubids, puboid)) then what is the point to
even assign/check aschemaPubids?

~~~

3.
+ if (is_except)
+ ereport(ERROR,
+ (errcode(ERRCODE_DUPLICATE_OBJECT),
+ errmsg("table \"%s\" cannot be added because it is excluded from
publication \"%s\"",
+ RelationGetQualifiedRelationName(targetrel),
+ pub->name)));
+ else
+ ereport(ERROR,
+ (errcode(ERRCODE_DUPLICATE_OBJECT),
+ errmsg("relation \"%s\" is already member of publication \"%s\"",
+ RelationGetRelationName(targetrel), pub->name)));

Fully qualified 'targetrel' in the first error, but not in the second? Is it OK?

~~~

GetAllSchemaPublicationRelations:

4.
+ List *exceptlist = NIL;

The varname is a bit vague; it is a list of "what"? Maybe say
'except_relids' or similar.

======
src/backend/replication/pgoutput/pgoutput.c

get_rel_sync_entry:

5.
+ /*
+ * For the schema EXCEPT check, we must look up the top-most ancestor
+ * rather than the relation itself. check_publication_add_relation()
+ * prevents individual partitions from appearing in the EXCEPT clause,
+ * so only a root (non-partition) table can have prexcept = true.
+ * Using the partition's own OID would always return NIL and miss the
+ * exclusion.
+ */
+ Oid root_relid = am_partition ?
+ llast_oid(get_partition_ancestors(relid)) : relid;
+
+ schemaExceptPubids = GetRelationExcludedPublications(root_relid);

5a.
The varname 'schemaExceptPubids' seems ambiguous. It sounds like it is
pubids that have EXCEPT SCHEMA. In the future the ALL TABLES may
introduce "EXCEPT SCHEMA", but currently there is no such thing.
Meanwhile, here I think it means "EXCEPT TABLE", so IMO that varname
needs to indicate the meaning better.

~

5b.
Actually, this is becoming a GENERAL comment. There too many ways that
these EXCEPT tables are getting named, and it is causing confusion:
- except_pubids
- exceptlist
- exceptrelations
- exceptrels
- except_relid
- except_tables
- schemaExceptPubids

Can we standardize on some common names, to make all the code more consistent?

~

5c.
Previously, the result of 'get_partition_ancestors' was being freed,
but now it is not. I'm not sure how importatnt that is, because I
found other examples in PG source code also not freeing...

======
src/bin/psql/tab-complete.in.c

6.
+ else if (Matches("CREATE", "PUBLICATION", MatchAny, "FOR", "TABLES",
"IN", "SCHEMA", MatchAny, "EXCEPT", "(", "TABLE", MatchAnyN) &&
ends_with(prev_wd, ','))
+ COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_tables);

I'm not sure if this is working as intended.

When testing for multiple except tables I get results like:
----
test_pub=# create publication pub1 for tables IN SCHEMA myschema <TAB>
EXCEPT ( TABLE WITH (
test_pub=# create publication pub1 for tables IN SCHEMA myschema
except ( table <TAB>
test_pub=# create publication pub1 for tables IN SCHEMA myschema
except ( table myschema.t<TAB>
myschema.t1 myschema.t2 myschema.t3
test_pub=# create publication pub1 for tables IN SCHEMA myschema
except ( table myschema.t1,<TAB>
information_schema. myschema. public. t1
t2 t3
----

Note: it is offering suggstions for schema names outside of the
"myschema". Should this code be calling
Query_for_list_of_tables_in_schema instead of
Query_for_list_of_tables?

======
src/test/regress/sql/publication.sql

7.
---- Tests for publications with SEQUENCES
+---------------------------------------------
+-- EXCEPT tests for TABLES IN SCHEMA
+---------------------------------------------
+SET client_min_messages = 'ERROR';

It looks like a previous comment for the SEQUENCES tests has been
accidentally removed.

I should be put back, and made more prominent like the other big comments.
e.g.
---------------------------------------------
-- Tests for publications with SEQUENCES
---------------------------------------------

~~~

8.
+RESET client_min_messages;
+DROP TABLE pub_test.testpub_tbl_s1, pub_test.testpub_tbl_s2;
+DROP TABLE pub_test.testpub_parted_s CASCADE;
+DROP TABLE testpub_nopk, testpub_tbl_s1;
+DROP PUBLICATION testpub_schema_except1, testpub_schema_except2,
testpub_schema_except_multi;
+

Add a "Cleanup" comment.

======
Kind Regards,
Peter Smith.
Fujitsu Australia.