getting ERROR "relation 16401 has no triggers" with partition foreign key alter
Hi,
I am getting ERROR: relation 16401 has no triggers error while executing
below query.
postgres=# create table tbl1(f1 int primary key);
CREATE TABLE
postgres=# create table tbl2(f1 int references tbl1 deferrable initially
deferred) partition by range(f1);
CREATE TABLE
postgres=# create table tbl2_p1 partition of tbl2 for values from
(minvalue) to (maxvalue);
CREATE TABLE
postgres=# insert into tbl1 values(1);
INSERT 0 1
postgres=# begin;
BEGIN
postgres=# insert into tbl2 values(1);
INSERT 0 1
postgres=# alter table tbl2 drop constraint tbl2_f1_fkey;
ALTER TABLE
postgres=# commit;
ERROR: relation 16395 has no triggers
Thanks & Regards,
Rajkumar Raghuwanshi
QMG, EnterpriseDB Corporation
On Tue, Jul 16, 2019 at 01:07:45PM +0530, Rajkumar Raghuwanshi wrote:
I am getting ERROR: relation 16401 has no triggers error while executing
below query.
Indeed, confirmed. I can reproduce that down to v11, so that's not an
open item. I have added an entry in the section for older issues
though.
--
Michael
Rajkumar Raghuwanshi <rajkumar.raghuwanshi@enterprisedb.com> writes:
I am getting ERROR: relation 16401 has no triggers error while executing
below query.
Yeah, I can reproduce that back to v11. If you try the same scenario
with a non-partitioned table you get
ERROR: 55006: cannot ALTER TABLE "tbl2" because it has pending trigger events
LOCATION: CheckTableNotInUse, tablecmds.c:3436
but that test evidently fails to detect pending events for a partition
child table.
regards, tom lane
On 2019-Jul-16, Tom Lane wrote:
Rajkumar Raghuwanshi <rajkumar.raghuwanshi@enterprisedb.com> writes:
I am getting ERROR: relation 16401 has no triggers error while executing
below query.Yeah, I can reproduce that back to v11. If you try the same scenario
with a non-partitioned table you getERROR: 55006: cannot ALTER TABLE "tbl2" because it has pending trigger events
LOCATION: CheckTableNotInUse, tablecmds.c:3436but that test evidently fails to detect pending events for a partition
child table.
Ah, yeah. So the problem is that when dropping an FK,
ATExecDropConstraint does not recurse itself, but instead relies on the
dependency mechanism, which obviously does not run CheckTableNotInUse on
the partitions.
I think we should just run CheckTableNotInUse for each partition in
ATExecDropConstraint. Trying that out now.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2019-Jul-17, Alvaro Herrera wrote:
I think we should just run CheckTableNotInUse for each partition in
ATExecDropConstraint. Trying that out now.
Actually, that doesn't fix this problem, because the partitioned side is
the *referencing* side, and ATExecDropConstraint is obsessed about the
*referenced* side only and assumes that the calling code has already
dealt with the referencing side checks. I'm trying a fix for that now.
I wonder if there are other AT subcommands that are similarly broken,
because many of them skip the CheckTableNotInUse for the partitions.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2019-Jul-17, Alvaro Herrera wrote:
Actually, that doesn't fix this problem, because the partitioned side is
the *referencing* side, and ATExecDropConstraint is obsessed about the
*referenced* side only and assumes that the calling code has already
dealt with the referencing side checks. I'm trying a fix for that now.
Yeah, the attached patch fixes Rajkumar's reproducer.
I wonder if there are other AT subcommands that are similarly broken,
because many of them skip the CheckTableNotInUse for the partitions.
I suppose the question here is where else do we need to call the new
ATRecurseCheckNotInUse function (which needs a comment).
I thought about doing the recursion in CheckTableNotInUse itself, but I
didn't feel comfortable with assuming that all callers are OK with that.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
recurse-dropfk.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index fc1c4dfa4c..a9dc2e05ca 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -354,6 +354,7 @@ static void ATSimplePermissions(Relation rel, int allowed_targets);
static void ATWrongRelkindError(Relation rel, int allowed_targets);
static void ATSimpleRecursion(List **wqueue, Relation rel,
AlterTableCmd *cmd, bool recurse, LOCKMODE lockmode);
+static void ATRecurseCheckNotInUse(Relation rel, LOCKMODE lockmode);
static void ATTypedTableRecursion(List **wqueue, Relation rel, AlterTableCmd *cmd,
LOCKMODE lockmode);
static List *find_typed_table_dependencies(Oid typeOid, const char *typeName,
@@ -3421,8 +3422,7 @@ CheckTableNotInUse(Relation rel, const char *stmt)
ereport(ERROR,
(errcode(ERRCODE_OBJECT_IN_USE),
/* translator: first %s is a SQL command, eg ALTER TABLE */
- errmsg("cannot %s \"%s\" because "
- "it is being used by active queries in this session",
+ errmsg("cannot %s \"%s\" because it is being used by active queries in this session",
stmt, RelationGetRelationName(rel))));
if (rel->rd_rel->relkind != RELKIND_INDEX &&
@@ -3431,8 +3431,7 @@ CheckTableNotInUse(Relation rel, const char *stmt)
ereport(ERROR,
(errcode(ERRCODE_OBJECT_IN_USE),
/* translator: first %s is a SQL command, eg ALTER TABLE */
- errmsg("cannot %s \"%s\" because "
- "it has pending trigger events",
+ errmsg("cannot %s \"%s\" because it has pending trigger events",
stmt, RelationGetRelationName(rel))));
}
@@ -3985,7 +3984,8 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
break;
case AT_DropConstraint: /* DROP CONSTRAINT */
ATSimplePermissions(rel, ATT_TABLE | ATT_FOREIGN_TABLE);
- /* Recursion occurs during execution phase */
+ ATRecurseCheckNotInUse(rel, lockmode);
+ /* Other recursion occurs during execution phase */
/* No command-specific prep needed except saving recurse flag */
if (recurse)
cmd->subtype = AT_DropConstraintRecurse;
@@ -5224,8 +5224,9 @@ ATSimpleRecursion(List **wqueue, Relation rel,
AlterTableCmd *cmd, bool recurse, LOCKMODE lockmode)
{
/*
- * Propagate to children if desired. Only plain tables and foreign tables
- * have children, so no need to search for other relkinds.
+ * Propagate to children if desired. Only plain tables, foreign tables
+ * and partitioned tables have children, so no need to search for other
+ * relkinds.
*/
if (recurse &&
(rel->rd_rel->relkind == RELKIND_RELATION ||
@@ -5259,6 +5260,26 @@ ATSimpleRecursion(List **wqueue, Relation rel,
}
}
+static void
+ATRecurseCheckNotInUse(Relation rel, LOCKMODE lockmode)
+{
+ if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+ {
+ List *inh;
+ ListCell *cell;
+
+ inh = find_all_inheritors(RelationGetRelid(rel), lockmode, NULL);
+ foreach(cell, inh)
+ {
+ Relation childrel = table_open(lfirst_oid(cell), NoLock);
+
+ CheckTableNotInUse(childrel, "ALTER TABLE");
+ table_close(childrel, NoLock);
+ }
+ list_free(inh);
+ }
+}
+
/*
* ATTypedTableRecursion
*
On 2019-Jul-17, Alvaro Herrera wrote:
On 2019-Jul-17, Alvaro Herrera wrote:
I wonder if there are other AT subcommands that are similarly broken,
because many of them skip the CheckTableNotInUse for the partitions.I suppose the question here is where else do we need to call the new
ATRecurseCheckNotInUse function (which needs a comment).
I decided to rename the new function to ATCheckPartitionsNotInUse, and
make it a no-op for legacy inheritance. This seems quite specific to
partitioned tables (as opposed to legacy inheritance behavior).
After looking at the code some more, I think calling the new function in
the Prep phase is correct. The attached patch is pretty much final form
for this bugfix. I decided to unwrap a couple of error messages (I did
get bitten while grepping because of this), and reordered one of the new
Identity command cases in ATPrepCmd since it appeared in inconsistent
order in that one place of four.
I looked at all the other AT subcommand cases that might require the
same treatment, and didn't find anything -- either the recursion is done
at Prep time, which checks already, or contains the proper check at Exec
time right after opening the partition rel. (I think it would be better
to do the check during the Prep phase, to avoid wasting work in case a
partition happens to be used. However, that's not critical and not for
this commit to fix IMO.)
Separately from that, there's AT_SetLogged / AT_SetUnlogged which look
pretty dubious ... I'm not sure that recursion is handled correctly
there. Maybe it's considered okay to have a partitioned table with
unlogged partitions, and vice versa?
I also noticed that AT_AlterConstraint does not handle recursion at all,
and it also has this comment:
* Currently only works for Foreign Key constraints.
* Foreign keys do not inherit, so we purposely ignore the
* recursion bit here, but we keep the API the same for when
* other constraint types are supported.
which sounds to oppose reality.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
v2-0001-Check-partitions-not-in-use.patchtext/x-diff; charset=us-asciiDownload
From ca7841a24c27012e351d08dc36039233ae0d80e1 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Mon, 22 Jul 2019 11:09:06 -0400
Subject: [PATCH v2] Check partitions not in use
---
src/backend/commands/tablecmds.c | 56 +++++++++++++++++++++++++-------
1 file changed, 45 insertions(+), 11 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index f39f993e01..fd17aa4b5a 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -354,6 +354,7 @@ static void ATSimplePermissions(Relation rel, int allowed_targets);
static void ATWrongRelkindError(Relation rel, int allowed_targets);
static void ATSimpleRecursion(List **wqueue, Relation rel,
AlterTableCmd *cmd, bool recurse, LOCKMODE lockmode);
+static void ATCheckPartitionsNotInUse(Relation rel, LOCKMODE lockmode);
static void ATTypedTableRecursion(List **wqueue, Relation rel, AlterTableCmd *cmd,
LOCKMODE lockmode);
static List *find_typed_table_dependencies(Oid typeOid, const char *typeName,
@@ -3421,8 +3422,7 @@ CheckTableNotInUse(Relation rel, const char *stmt)
ereport(ERROR,
(errcode(ERRCODE_OBJECT_IN_USE),
/* translator: first %s is a SQL command, eg ALTER TABLE */
- errmsg("cannot %s \"%s\" because "
- "it is being used by active queries in this session",
+ errmsg("cannot %s \"%s\" because it is being used by active queries in this session",
stmt, RelationGetRelationName(rel))));
if (rel->rd_rel->relkind != RELKIND_INDEX &&
@@ -3431,8 +3431,7 @@ CheckTableNotInUse(Relation rel, const char *stmt)
ereport(ERROR,
(errcode(ERRCODE_OBJECT_IN_USE),
/* translator: first %s is a SQL command, eg ALTER TABLE */
- errmsg("cannot %s \"%s\" because "
- "it has pending trigger events",
+ errmsg("cannot %s \"%s\" because it has pending trigger events",
stmt, RelationGetRelationName(rel))));
}
@@ -3910,16 +3909,19 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
break;
case AT_AddIdentity:
ATSimplePermissions(rel, ATT_TABLE | ATT_VIEW | ATT_FOREIGN_TABLE);
+ /* This command never recurses */
pass = AT_PASS_ADD_CONSTR;
break;
- case AT_DropIdentity:
- ATSimplePermissions(rel, ATT_TABLE | ATT_VIEW | ATT_FOREIGN_TABLE);
- pass = AT_PASS_DROP;
- break;
case AT_SetIdentity:
ATSimplePermissions(rel, ATT_TABLE | ATT_VIEW | ATT_FOREIGN_TABLE);
+ /* This command never recurses */
pass = AT_PASS_COL_ATTRS;
break;
+ case AT_DropIdentity:
+ ATSimplePermissions(rel, ATT_TABLE | ATT_VIEW | ATT_FOREIGN_TABLE);
+ /* This command never recurses */
+ pass = AT_PASS_DROP;
+ break;
case AT_DropNotNull: /* ALTER COLUMN DROP NOT NULL */
ATSimplePermissions(rel, ATT_TABLE | ATT_FOREIGN_TABLE);
ATPrepDropNotNull(rel, recurse, recursing);
@@ -3985,7 +3987,8 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
break;
case AT_DropConstraint: /* DROP CONSTRAINT */
ATSimplePermissions(rel, ATT_TABLE | ATT_FOREIGN_TABLE);
- /* Recursion occurs during execution phase */
+ ATCheckPartitionsNotInUse(rel, lockmode);
+ /* Other recursion occurs during execution phase */
/* No command-specific prep needed except saving recurse flag */
if (recurse)
cmd->subtype = AT_DropConstraintRecurse;
@@ -5224,8 +5227,9 @@ ATSimpleRecursion(List **wqueue, Relation rel,
AlterTableCmd *cmd, bool recurse, LOCKMODE lockmode)
{
/*
- * Propagate to children if desired. Only plain tables and foreign tables
- * have children, so no need to search for other relkinds.
+ * Propagate to children if desired. Only plain tables, foreign tables
+ * and partitioned tables have children, so no need to search for other
+ * relkinds.
*/
if (recurse &&
(rel->rd_rel->relkind == RELKIND_RELATION ||
@@ -5259,6 +5263,36 @@ ATSimpleRecursion(List **wqueue, Relation rel,
}
}
+/*
+ * Obtain list of partitions of the given table, locking them all at the given
+ * lockmode and ensuring that they all pass CheckTableNotInUse.
+ *
+ * This function is a no-op if the given relation is not a partitioned table;
+ * in particular, nothing is done if it's a legacy inheritance parent.
+ */
+static void
+ATCheckPartitionsNotInUse(Relation rel, LOCKMODE lockmode)
+{
+ if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+ {
+ List *inh;
+ ListCell *cell;
+
+ inh = find_all_inheritors(RelationGetRelid(rel), lockmode, NULL);
+ /* first element is the parent rel; must ignore it */
+ for_each_cell(cell, inh, list_second_cell(inh))
+ {
+ Relation childrel;
+
+ /* find_all_inheritors already got lock */
+ childrel = table_open(lfirst_oid(cell), NoLock);
+ CheckTableNotInUse(childrel, "ALTER TABLE");
+ table_close(childrel, NoLock);
+ }
+ list_free(inh);
+ }
+}
+
/*
* ATTypedTableRecursion
*
--
2.17.1
On 2019-Jul-22, Alvaro Herrera wrote:
After looking at the code some more, I think calling the new function in
the Prep phase is correct. The attached patch is pretty much final form
for this bugfix. I decided to unwrap a couple of error messages (I did
get bitten while grepping because of this), and reordered one of the new
Identity command cases in ATPrepCmd since it appeared in inconsistent
order in that one place of four.
Pushed to all three branches.
Thanks for reporting
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
On 2019-Jul-22, Alvaro Herrera wrote:
After looking at the code some more, I think calling the new function in
the Prep phase is correct. The attached patch is pretty much final form
for this bugfix. I decided to unwrap a couple of error messages (I did
get bitten while grepping because of this), and reordered one of the new
Identity command cases in ATPrepCmd since it appeared in inconsistent
order in that one place of four.
Pushed to all three branches.
This is still listed as a live issue in
https://wiki.postgresql.org/wiki/PostgreSQL_12_Open_Items#Live_issues
Should that be closed now?
regards, tom lane
On 2019-Aug-14, Tom Lane wrote:
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
On 2019-Jul-22, Alvaro Herrera wrote:
After looking at the code some more, I think calling the new function in
the Prep phase is correct. The attached patch is pretty much final form
for this bugfix. I decided to unwrap a couple of error messages (I did
get bitten while grepping because of this), and reordered one of the new
Identity command cases in ATPrepCmd since it appeared in inconsistent
order in that one place of four.Pushed to all three branches.
This is still listed as a live issue in
https://wiki.postgresql.org/wiki/PostgreSQL_12_Open_Items#Live_issues
Should that be closed now?
Yep, done, thanks!
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services