On Thu, 15 Apr 2021 at 21:24, Justin Pryzby <pry...@telsasoft.com> wrote:
>
> On Thu, Apr 15, 2021 at 08:47:26PM +0200, Matthias van de Meent wrote:
> > I recently noticed that ATTACH PARTITION also recursively locks the
> > default partition with ACCESS EXCLUSIVE mode when its constraints do
> > not explicitly exclude the to-be-attached partition, which I couldn't
> > find documented (has been there since PG10 I believe).
>
> I'm not sure it's what you're looking for, but maybe you saw:
> https://www.postgresql.org/docs/12/sql-altertable.html
> |The default partition can't contain any rows that would need to be moved to
> the
> |new partition, and will be scanned to verify that none are present. This
> scan,
> |like the scan of the new partition, can be avoided if an appropriate
> |<literal>CHECK</literal> constraint is present.
>
> And since 2a4d96ebb:
> |Attaching a partition acquires a SHARE UPDATE EXCLUSIVE lock on the parent
> table, in addition to ACCESS EXCLUSIVE locks on the table to be attached and
> on the default partition (if any).
>From the current documentation the recursive locking isn't clear: I
didn't expect an ACCESS EXCLUSIVE on the whole hierarchy of both the
to-be-attached and the default partitions whilst scanning, because the
SUEL on the shared parent is not propagated to all its children
either.
> From your patch:
>
> > + <para>
> > + Similarly, if you have a default partition on the parent table, it is
> > + recommended to create a <literal>CHECK</literal> constraint that
> > excludes
> > + the to be attached partition constraint. Here, too, without the
> > + <literal>CHECK</literal> constraint, this table will be scanned to
> > + validate that the updated default partition constraints while holding
> > + an <literal>ACCESS EXCLUSIVE</literal> lock on the default partition.
> > + </para>
>
> The AEL is acquired in any case, right ?
Yes, the main point is that the validation scan runs whilst holding
the AEL on the partition (sub)tree of that default partition. After
looking at bit more at the code, I agree that my current patch is not
descriptive enough.
I compared adding a partition to running `CREATE CONSTRAINT ... NOT
VALID` on the to-be-altered partitions (using AEL), + `VALIDATE
CONSTRAINT` running recursively over it's partitions (using SHARE
UPDATE EXCLUSIVE). We only expect an SUEL for VALIDATE CONSTRAINT, and
the constraint itself is only added/updated to the direct descendents
of the parent, not their recursivedescendents. Insertions already can
only happen when the whole upward hierarchy of a partition allows for
inserts, so this shouldn't be that much of an issue.
> I think whatever we say here needs to be crystal clear that only the scan can
> be skipped.
Yes, but when we skip the scan for the default partition, we also skip
locking its partition tree with AEL. The partition tree of the table
that is being attached, however, is fully locked regardless of
constraint definitions.
> I suggest that maybe the existing paragraph in alter_table.sgml could maybe
> say
> that an exclusive lock is held, maybe like.
>
> |The default partition can't contain any rows that would need to be moved to
> the
> |new partition, and will be scanned to verify that none are present. This
> scan,
> |like the scan of the new partition, can be avoided if an appropriate
> |<literal>CHECK</literal> constraint is present.
> |The scan of the default partition occurs while it is exclusively locked.
PFA an updated patch. I've updated the wording of the previous patch,
and also updated this section in alter_table.sgml, but with different
wording, explictly explaining the process used to validate the altered
default constraint.
Thanks for the review.
With regards,
Matthias van de Meent
From 230c594950886bf50bf4c69295aa0cb67c16b8a6 Mon Sep 17 00:00:00 2001
From: Matthias van de Meent <boekew...@gmail.com>
Date: Thu, 15 Apr 2021 20:43:00 +0200
Subject: [PATCH v2] ATTACH PARTITION locking documentation for DEFAULT
partitions.
---
doc/src/sgml/ddl.sgml | 18 ++++++++++++++++--
doc/src/sgml/ref/alter_table.sgml | 8 ++++++--
2 files changed, 22 insertions(+), 4 deletions(-)
diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index 30e4170963..8f0b38847a 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -3934,6 +3934,9 @@ ALTER TABLE measurement_y2008m02 ADD CONSTRAINT y2008m02
\copy measurement_y2008m02 from 'measurement_y2008m02'
-- possibly some other data preparation work
+ALTER TABLE measurement_default ADD CONSTRAINT excl_y2008m02
+ CHECK ( (logdate >= DATE '2008-02-01' AND logdate < DATE '2008-03-01') IS FALSE );
+
ALTER TABLE measurement ATTACH PARTITION measurement_y2008m02
FOR VALUES FROM ('2008-02-01') TO ('2008-03-01' );
</programlisting>
@@ -3947,12 +3950,23 @@ ALTER TABLE measurement ATTACH PARTITION measurement_y2008m02
which is otherwise needed to validate the implicit
partition constraint. Without the <literal>CHECK</literal> constraint,
the table will be scanned to validate the partition constraint while
- holding both an <literal>ACCESS EXCLUSIVE</literal> lock on that partition
- and a <literal>SHARE UPDATE EXCLUSIVE</literal> lock on the parent table.
+ holding an <literal>ACCESS EXCLUSIVE</literal> lock on that partition
+ and the tables in its downward partition hierarchy (if any), and a
+ <literal>SHARE UPDATE EXCLUSIVE</literal> lock on the parent table.
It is recommended to drop the now-redundant <literal>CHECK</literal>
constraint after <command>ATTACH PARTITION</command> is finished.
</para>
+ <para>
+ Similarly, if you have a default partition on the parent table, it is
+ recommended to create a <literal>CHECK</literal> constraint that excludes
+ the to be attached partition constraint. If the default partition does
+ not exclude the to be attached partition constraint, the partition
+ hierarchy of the default partition will recursively be checked against
+ the updated partition bounds whilst holding an
+ <literal>ACCESS EXCLUSIVE</literal> lock.
+ </para>
+
<para>
As explained above, it is possible to create indexes on partitioned tables
so that they are applied automatically to the entire hierarchy.
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 07e37a6dc8..2801d87827 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -947,8 +947,12 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
<para>
Attaching a partition acquires a
<literal>SHARE UPDATE EXCLUSIVE</literal> lock on the parent table,
- in addition to <literal>ACCESS EXCLUSIVE</literal> locks on the table
- to be attached and on the default partition (if any).
+ in addition to <literal>ACCESS EXCLUSIVE</literal> locks on the full
+ partition hierarchy of the to be attached table and on the default
+ partition (if any). If the constraints of the default partition do not
+ already imply the new partition bounds, its descendants will
+ recursively be locked with an <literal>ACCESS EXCLUSIVE</literal> lock
+ and validated against the updated constraints.
</para>
</listitem>
</varlistentry>
--
2.20.1
