BUG #17339: Assert failed on attempt to detach a sequence concurrently

On Sun, Dec 19, 2021 at 06:00:02AM +0000, PG Bug reporting form wrote:

CREATE SEQUENCE seq;
CREATE TABLE range_parted(a int) PARTITION BY RANGE(a);
ALTER TABLE range_parted DETACH PARTITION seq CONCURRENTLY;

The same error happens additionally for views or materialized views.
Looking at the code, I think that we should just apply
ATSimplePermissions() on (ATT_TABLE | ATT_FOREIGN_TABLE) when
executing the detach command to check for the supported relkinds.
That would make the logic consistent with the attach code path that
does the same check on the partition attached, while generating an
error message already generic enough for this purpose.

Attached is a patch, with some regression tests.
--
Michael

On 20.12.21 13:38, Michael Paquier wrote:

On Sun, Dec 19, 2021 at 06:00:02AM +0000, PG Bug reporting form wrote:

CREATE SEQUENCE seq;
CREATE TABLE range_parted(a int) PARTITION BY RANGE(a);
ALTER TABLE range_parted DETACH PARTITION seq CONCURRENTLY;

The same error happens additionally for views or materialized views.
Looking at the code, I think that we should just apply
ATSimplePermissions() on (ATT_TABLE | ATT_FOREIGN_TABLE) when
executing the detach command to check for the supported relkinds.
That would make the logic consistent with the attach code path that
does the same check on the partition attached, while generating an
error message already generic enough for this purpose.

Attached is a patch, with some regression tests.

Is it possible for child tables in partitioned tables to have different
ownerships? Then this change would introduce a new failure mode.

On Mon, Dec 20, 2021 at 09:57:47PM +0100, Peter Eisentraut wrote:

On 20.12.21 13:38, Michael Paquier wrote:

Attached is a patch, with some regression tests.

Is it possible for child tables in partitioned tables to have different
ownerships? Then this change would introduce a new failure mode.

Hmm. Yes, you are right here. It is possible to change the ownership
of a partition after it gets attached, so this could cause a
regression. I recalled that this was not possible, so my memories
were wrong.

At the end, perhaps we should just remove the assertion that assumes
which relkind is right for the partition, as of:
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -15013,8 +15013,6 @@ MarkInheritDetached(Relation child_rel,
Relation parent_rel)
    HeapTuple   inheritsTuple;
        bool        found = false;

- Assert(child_rel->rd_rel->relkind == RELKIND_RELATION ||
- child_rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE);
Assert(parent_rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE);

This would also fail when attempting to detach a foreign table, as
well, and these are legal relkinds in a partition tree. Once we do
that, we fall down into the same failure as for the non-concurrent
mode in RemoveInheritance(), telling that the relation is not a member
of the partition tree.
--
Michael

At Tue, 21 Dec 2021 09:30:42 +0900, Michael Paquier <michael@paquier.xyz> wrote in

On Mon, Dec 20, 2021 at 09:57:47PM +0100, Peter Eisentraut wrote:

On 20.12.21 13:38, Michael Paquier wrote:

Attached is a patch, with some regression tests.

Is it possible for child tables in partitioned tables to have different
ownerships? Then this change would introduce a new failure mode.

Hmm. Yes, you are right here. It is possible to change the ownership
of a partition after it gets attached, so this could cause a
regression. I recalled that this was not possible, so my memories
were wrong.

At the end, perhaps we should just remove the assertion that assumes
which relkind is right for the partition, as of:
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -15013,8 +15013,6 @@ MarkInheritDetached(Relation child_rel,
Relation parent_rel)
HeapTuple   inheritsTuple;
bool        found = false;

- Assert(child_rel->rd_rel->relkind == RELKIND_RELATION ||
- child_rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE);
Assert(parent_rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE);

This would also fail when attempting to detach a foreign table, as
well, and these are legal relkinds in a partition tree. Once we do
that, we fall down into the same failure as for the non-concurrent
mode in RemoveInheritance(), telling that the relation is not a member
of the partition tree.

I agree to the discussion above thus it seems to me right that we just
remove the assertion. We never have a partition of un-attachable
relkind so such kind of relations are surely rejected just after. The
assertion on "== RELKIND_PARTITIONED_TABLE" is still valid since we
don't reach there for the case of the traditional inheritance.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Hello Michael and Kyotaro-san,
21.12.2021 06:03, Kyotaro Horiguchi wrote:

This would also fail when attempting to detach a foreign table, as
well, and these are legal relkinds in a partition tree. Once we do
that, we fall down into the same failure as for the non-concurrent
mode in RemoveInheritance(), telling that the relation is not a member
of the partition tree.

I agree to the discussion above thus it seems to me right that we just
remove the assertion. We never have a partition of un-attachable
relkind so such kind of relations are surely rejected just after. The
assertion on "== RELKIND_PARTITIONED_TABLE" is still valid since we
don't reach there for the case of the traditional inheritance.

As detaching a foreign table is a valid scenario (not covered before),
maybe it's worth to exercise it as following:

--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -1490,6 +1490,8 @@ CREATE FOREIGN TABLE foreign_tbl (a int, b int)
 
 CREATE TABLE parent_tbl (a int, b int) PARTITION BY RANGE(a);
 ALTER TABLE parent_tbl ATTACH PARTITION foreign_tbl FOR VALUES FROM (0)
TO (100);
+ALTER TABLE parent_tbl DETACH PARTITION foreign_tbl CONCURRENTLY;
+ALTER TABLE parent_tbl ATTACH PARTITION foreign_tbl FOR VALUES FROM (0)
TO (100);
 
 CREATE VIEW rw_view AS SELECT * FROM parent_tbl
   WHERE a < b WITH CHECK OPTION;

Thanks!

Best regards,
Alexander

On Tue, Dec 21, 2021 at 07:00:00AM +0300, Alexander Lakhin wrote:

As detaching a foreign table is a valid scenario (not covered before),
maybe it's worth to exercise it as following:

Yeah, I was going to add something in the main regression test suite
as of foreign_data.sql, but the detached partitions are used with
constraint checks so that was a bit confusing. What you are
suggesting is fine by me, so I have included these and pushed a fix
for the issue.
--
Michael