Thread: ATTACH PARTITION locking documentation for DEFAULT partitions

Hi,

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).

PFA a patch that documents just that.

With regards,

Matthias van de Meent.

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
lockson the table to be attached and on the default partition (if any).
 

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 ?

I think whatever we say here needs to be crystal clear that only the scan can
be skipped.

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.

-- 
Justin

On Thu, 15 Apr 2021 at 21:24, Justin Pryzby <pryzby@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
lockson 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

On Sat, 17 Apr 2021 at 00:03, Matthias van de Meent
<boekewurm+postgres@gmail.com> wrote:
> 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.

I had to squint at this:

+ALTER TABLE measurement_default ADD CONSTRAINT excl_y2008m02
+   CHECK ( (logdate >= DATE '2008-02-01' AND logdate < DATE
'2008-03-01') IS FALSE );

I tried your example and it does not work.

set client_min_messages = 'debug1';
create table rp (dt date not null) partition by range(dt);
create table rp_default partition of rp default;
alter table rp_default add constraint rp_default_chk check ((dt >=
'2022-01-01' and dt < '2023-01-01') is false);
create table rp_2022 partition of rp for values from ('2022-01-01') to
('2023-01-01');

There's no debug message to indicate that the constraint was used.

Let's try again:

alter table rp_default drop constraint rp_default_chk;
drop table rp_2022;
alter table rp_default add constraint rp_default_chk check (not (dt >=
'2022-01-01' and dt < '2023-01-01'));
create table rp_2022 partition of rp for values from ('2022-01-01') to
('2023-01-01');
DEBUG:  updated partition constraint for default partition
"rp_default" is implied by existing constraints

The debug message indicates that it worked as expected that time.

But to be honest, I don't know why you've even added that. There's not
even an example on how to add a DEFAULT partition, so why should we
include an example of how to add a CHECK constraint on one?

I've spent a bit of time hacking at this and I've come up with the
attached patch.

David

On Mon, 5 Jul 2021 at 01:01, David Rowley <dgrowleyml@gmail.com> wrote:
> I've spent a bit of time hacking at this and I've come up with the
> attached patch.

Matthias, any thoughts on my revised version of the patch?

David

On Mon, 12 Jul 2021 at 14:06, David Rowley <dgrowleyml@gmail.com> wrote:
>
> On Mon, 5 Jul 2021 at 01:01, David Rowley <dgrowleyml@gmail.com> wrote:
> > I've spent a bit of time hacking at this and I've come up with the
> > attached patch.
>
> Matthias, any thoughts on my revised version of the patch?

Sorry for the delay. I think that  covers the basics of what I was
missing in these docs, and although it does not cover the recursive
'if the check is implied by constraints don't lock this partition',
I'd say that your suggested patch is good enough. Thanks for looking
over this.

Kind regards,

Matthias van de Meent

On Tue, 13 Jul 2021 at 00:14, Matthias van de Meent
<boekewurm+postgres@gmail.com> wrote:
> Sorry for the delay. I think that  covers the basics of what I was
> missing in these docs, and although it does not cover the recursive
> 'if the check is implied by constraints don't lock this partition',
> I'd say that your suggested patch is good enough. Thanks for looking
> over this.

Isn't that covered the following?

+     <para>
+      Further locks must also be held on all sub-partitions if the table being
+      attached is itself a partitioned table.  Likewise if the default
+      partition is itself a partitioned table.  The locking of the
+      sub-partitions can be avoided by adding a <literal>CHECK</literal>
+      constraint as described in
+      <xref linkend="ddl-partitioning-declarative-maintenance"/>.
      </para>

David

On Mon, 12 Jul 2021 at 15:28, David Rowley <dgrowleyml@gmail.com> wrote:
>
> On Tue, 13 Jul 2021 at 00:14, Matthias van de Meent
> <boekewurm+postgres@gmail.com> wrote:
> > Sorry for the delay. I think that  covers the basics of what I was
> > missing in these docs, and although it does not cover the recursive
> > 'if the check is implied by constraints don't lock this partition',
> > I'd say that your suggested patch is good enough. Thanks for looking
> > over this.
>
> Isn't that covered the following?
>
> +     <para>
> +      Further locks must also be held on all sub-partitions if the table being
> +      attached is itself a partitioned table.  Likewise if the default
> +      partition is itself a partitioned table.  The locking of the
> +      sub-partitions can be avoided by adding a <literal>CHECK</literal>
> +      constraint as described in
> +      <xref linkend="ddl-partitioning-declarative-maintenance"/>.
>       </para>

The exact behaviour is (c.q. QueuePartitionConstraintValidation in
tablecmds.c:17072), for each partition of this table:

1.) if the existing constraints imply the new constraints: return to .
2.) lock this partition with ACCESS EXCLUSIVE
3.) if this is a partitioned table, for each direct child partition,
execute this algorithm.

The algoritm as described in your patch implies that this recursive
locking is conditional on _only_ the check-constraints of the topmost
partition ("performed whilst holding ... and all of its
sub-partitions, if any"), whereas actually the locking on each
(sub-)partition is determined by the constraints of the hierarchy down
to that child partition. It in actuality, this should not matter much,
but this is a meaningful distinction that I wanted to call out.

Regardless of the distinction between actual locking behaviour and
this documentation, we might not want to document this specific
algorithm, as this algorithm might be changed in future versions, and
the proposed documentation leaves a little wiggleroom in changing the
locking behaviour without needing to update the docs.

Kind regards,

Matthias van de Meent

On Tue, 13 Jul 2021 at 02:30, Matthias van de Meent
<boekewurm+postgres@gmail.com> wrote:
> The algoritm as described in your patch implies that this recursive
> locking is conditional on _only_ the check-constraints of the topmost
> partition ("performed whilst holding ... and all of its
> sub-partitions, if any"), whereas actually the locking on each
> (sub-)partition is determined by the constraints of the hierarchy down
> to that child partition. It in actuality, this should not matter much,
> but this is a meaningful distinction that I wanted to call out.

I had in mind that was implied, but maybe it's better to be explicit about that.

I've adjusted the patch and attached what I came up with. Let me know
what you think.

I think this can be back-patched as far as 12. Before then we took an
AEL on the partitioned table, so it seems much less important since
any concurrency would be blown out by the AEL.

David

On Tue, 27 Jul 2021 at 08:02, David Rowley <dgrowleyml@gmail.com> wrote:\>
> On Tue, 13 Jul 2021 at 02:30, Matthias van de Meent
> <boekewurm+postgres@gmail.com> wrote:
> > The algoritm as described in your patch implies that this recursive
> > locking is conditional on _only_ the check-constraints of the topmost
> > partition ("performed whilst holding ... and all of its
> > sub-partitions, if any"), whereas actually the locking on each
> > (sub-)partition is determined by the constraints of the hierarchy down
> > to that child partition. It in actuality, this should not matter much,
> > but this is a meaningful distinction that I wanted to call out.
>
> I had in mind that was implied, but maybe it's better to be explicit about that.
>
> I've adjusted the patch and attached what I came up with. Let me know
> what you think.

I like this improved wording. Thanks!

Kind regards,

Matthias van de Meent

On Tue, 27 Jul 2021 at 21:36, Matthias van de Meent
<boekewurm+postgres@gmail.com> wrote:
>
> On Tue, 27 Jul 2021 at 08:02, David Rowley <dgrowleyml@gmail.com> wrote:\>
> > I've adjusted the patch and attached what I came up with. Let me know
> > what you think.
>
> I like this improved wording. Thanks!

I've pushed this with some very minor further wording adjustments.

Thanks for working on this.

David