Re: ATTACH PARTITION locking documentation for DEFAULT partitions

On Thu, 15 Apr 2021 at 21:24, Justin Pryzby <pryzby(at)telsasoft(dot)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