Hello,
I think reimplementing list_member_oid() under a different name
(is_ancestor_member_relids) is pointless and should not be done.
It also appears to me that we haven't nailed the error messages just
yet. I tried to fix it upthread, but didn't really get it correct. For
instance, consider this case taken from the new regression tests:
/* test setup */
CREATE SCHEMA sch3;
CREATE TABLE sch3.tmain(id int) PARTITION BY RANGE(id);
CREATE TABLE sch3.part1 PARTITION OF sch3.tmain FOR VALUES FROM (0) TO (5);
CREATE TABLE sch3.part2(id int) PARTITION BY RANGE(id);
CREATE FOREIGN TABLE sch3.part2_1 PARTITION OF sch3.part2 FOR VALUES FROM (5) TO (10) SERVER fdw_server;
ALTER TABLE sch3.tmain ATTACH PARTITION sch3.part2 FOR VALUES FROM (5) TO (10);
The issue occurs in the next command:
CREATE PUBLICATION pub1 FOR TABLE sch3.tmain WITH (publish_via_partition_root);
ERROR: cannot add partitioned table "tmain" to publication "pub1", which has "publish_via_partition_root" set to
"true"
DETAIL: The table contains a partition that's a foreign table.
The user is trying to create a publication, so an error saying that it's
not possible to add a partition is a bit off-point. (If you squint hard
enough it sort of makes sense, but it seems inconsistent enough.)
We could replace the word "add" with the word "include" there and this
particular problem would be fixed, perhaps. However, the next one is
worse:
CREATE PUBLICATION pub1 FOR ALL TABLES WITH (publish_via_partition_root);
ERROR: cannot set parameter "publish_via_partition_root" to "true" for publication "pub1"
DETAIL: Published partitioned table "tmain" contains a partition that is a foreign table.
Here we again try to create a publication, but we get an error that we
cannot set the parameter, which is even more detached from reality.
Maybe what we need is to add another parameter to that function to
indicate which operation is being executed (create publication, add
schema, change the parameter), which is used to indicate the error
message to throw.
With that thought in mind, I think it would make things simpler for us
to remove the duplicate copies of the same error messages by adding yet
another function, after publication_check_foreign_parts(), which receives
the operation being executed and whatever additional parameter it needs,
and throws the error. If publication_check_foreign_parts() also
receives the operation being executed, it can simply pass it through to
that new function when an error needs to be thrown; other places that
hardcode error situations would pass a constant for the operation.
But the non-idiomatic locking of pg_partitioned_table appears to
continue to be the pain point of this patch. My impression is that
using a lock is the wrong approach to solve the concurrency problem.
Maybe we can use a ConditionVariable instead somehow. (The real trick
here is to figure out exactly _how_ to use the CV, I mean what exactly
is the condition that the CV sleeps on?)
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"Industry suffers from the managerial dogma that for the sake of stability
and continuity, the company should be independent of the competence of
individual employees." (E. Dijkstra)
