public inbox for [email protected]  

help / color / mirror / Atom feed

From: Álvaro Herrera <[email protected]>
To: Shlok Kyal <[email protected]>
Cc: vignesh C <[email protected]>
Cc: Sergey Tatarintsev <[email protected]>
Cc: [email protected]
Subject: Re: Restrict publishing of partitioned table with a foreign table as partition
Date: Sat, 10 May 2025 13:59:27 -0400
Message-ID: <[email protected]> (raw)
In-Reply-To: <CANhcyEVQ866NA5NbTi-aQhPj2zN_vbNb_ZW=E_P2foZYrW9VbA@mail.gmail.com>

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)

view thread (19+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: Restrict publishing of partitioned table with a foreign table as partition
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox