I wrote:
>> (2) ALTER TABLE ONLY ... ADD CONSTRAINT on a partition root tries to get
>> AccessExclusiveLock on all child partitions, despite the ONLY.
> The cause of this seems to be that ATPrepSetNotNull is too dumb to
> avoid recursing to all the child tables when the parent is already
> attnotnull. Or is there a reason we have to recurse anyway?
I wrote a quick patch for this part. It seems pretty safe and probably
could be back-patched without fear. (I also noticed that
ATSimpleRecursion is being unnecessarily stupid: instead of the
demonstrably not-future-proof relkind check, it could test relhassubclass,
which is not only simpler and less likely to need future changes, but
is able to save a scan of pg_inherits in a lot more cases.)
As far as I can tell in some quick testing, this fix is sufficient to
resolve the complained-of deadlock. It'd still be a good idea to fix the
TRUNCATE side of things as well. But that would be hard to back-patch
because removing ri_PartitionCheck, or even just failing to fill it,
seems like a potential ABI break for extensions. So my proposal is
to back-patch this, but address the ResultRelInfo change only in HEAD.
regards, tom lane
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index c21a309f04..4a51d79d09 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -5681,14 +5681,10 @@ ATSimpleRecursion(List **wqueue, Relation rel,
AlterTableUtilityContext *context)
{
/*
- * Propagate to children if desired. Only plain tables, foreign tables
- * and partitioned tables have children, so no need to search for other
- * relkinds.
+ * Propagate to children, if desired and if there are (or might be) any
+ * children.
*/
- if (recurse &&
- (rel->rd_rel->relkind == RELKIND_RELATION ||
- rel->rd_rel->relkind == RELKIND_FOREIGN_TABLE ||
- rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE))
+ if (recurse && rel->rd_rel->relhassubclass)
{
Oid relid = RelationGetRelid(rel);
ListCell *child;
@@ -6698,6 +6694,35 @@ ATPrepSetNotNull(List **wqueue, Relation rel,
if (recursing)
return;
+ /*
+ * If the target column is already marked NOT NULL, we can skip recursing
+ * to children, because their columns should already be marked NOT NULL as
+ * well. But there's no point in checking here unless the relation has
+ * some children; else we can just wait till execution to check. (If it
+ * does have children, however, this can save taking per-child locks
+ * unnecessarily. This greatly improves concurrency in some parallel
+ * restore scenarios.)
+ */
+ if (rel->rd_rel->relhassubclass)
+ {
+ HeapTuple tuple;
+ bool attnotnull;
+
+ tuple = SearchSysCacheAttName(RelationGetRelid(rel), cmd->name);
+
+ /* Might as well throw the error now, if name is bad */
+ if (!HeapTupleIsValid(tuple))
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_COLUMN),
+ errmsg("column \"%s\" of relation \"%s\" does not exist",
+ cmd->name, RelationGetRelationName(rel))));
+
+ attnotnull = ((Form_pg_attribute) GETSTRUCT(tuple))->attnotnull;
+ ReleaseSysCache(tuple);
+ if (attnotnull)
+ return;
+ }
+
/*
* If we have ALTER TABLE ONLY ... SET NOT NULL on a partitioned table,
* apply ALTER TABLE ... CHECK NOT NULL to every child. Otherwise, use
