Thread: Reduce lock level for ALTER TABLE ... ADD CHECK .. NOT VALID
897795240cfaaed724af2f53ed2c50c9862f951f forgot to reduce the lock level for CHECK constraints when allowing them to be NOT VALID. This is simple and safe, since check constraints are not used in planning until validated. -- Simon Riggs http://www.EnterpriseDB.com/
Attachment
The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: tested, passed Documentation: tested, passed Looks fine to me The new status of this patch is: Ready for Committer
On Thu, Apr 22, 2021 at 8:01 AM Simon Riggs <simon.riggs@enterprisedb.com> wrote:
>
> 897795240cfaaed724af2f53ed2c50c9862f951f forgot to reduce the lock
> level for CHECK constraints when allowing them to be NOT VALID.
>
> This is simple and safe, since check constraints are not used in
> planning until validated.
The patch also reduces the lock level when NOT VALID is not specified, which didn't seem to be the intention.
# begin;
BEGIN
*# alter table alterlock2 add check (f1 > 0);
ALTER TABLE
*# select * from my_locks order by 1;
relname | max_lockmode
------------+-----------------------
alterlock2 | ShareRowExclusiveLock
(1 row)
--
John Naylor
EDB: http://www.enterprisedb.com
BEGIN
*# alter table alterlock2 add check (f1 > 0);
ALTER TABLE
*# select * from my_locks order by 1;
relname | max_lockmode
------------+-----------------------
alterlock2 | ShareRowExclusiveLock
(1 row)
--
John Naylor
EDB: http://www.enterprisedb.com
On Sat, Jul 10, 2021 at 2:50 PM John Naylor <john.naylor@enterprisedb.com> wrote: > On Thu, Apr 22, 2021 at 8:01 AM Simon Riggs <simon.riggs@enterprisedb.com> wrote: > > > > 897795240cfaaed724af2f53ed2c50c9862f951f forgot to reduce the lock > > level for CHECK constraints when allowing them to be NOT VALID. > > > > This is simple and safe, since check constraints are not used in > > planning until validated. > > The patch also reduces the lock level when NOT VALID is not specified, which didn't seem to be the intention. Thank you for reviewing. I agree that the behavior works as you indicated. My description of this was slightly muddled. The lock level for CONSTR_FOREIGN applies whether or not NOT VALID is used, but the test case covers only NOT VALID because it a) isn't tested and b) is more important. I just followed that earlier pattern and that led me to adding "NOT VALID" onto the title of the thread. What is true for CONSTR_FOREIGN is also true for CONSTR_CHECK - the lock level can be set down to ShareRowExclusiveLock in all cases because adding a new CHECK does not affect the outcome of currently executing SELECT statements. (Note that this is not true for Drop Constraint, which has a different lock level, but we aren't changing that here). Once the constraint is validated it may influence the optimization of later SELECTs. So the patch and included docs are completely correct. Notice that the name of the patch reflects this better than the title of the thread. -- Simon Riggs http://www.EnterpriseDB.com/
On Thu, 15 Jul 2021 at 07:47, Simon Riggs <simon.riggs@enterprisedb.com> wrote: > > On Sat, Jul 10, 2021 at 2:50 PM John Naylor > <john.naylor@enterprisedb.com> wrote: > > On Thu, Apr 22, 2021 at 8:01 AM Simon Riggs <simon.riggs@enterprisedb.com> wrote: > > > > > > 897795240cfaaed724af2f53ed2c50c9862f951f forgot to reduce the lock > > > level for CHECK constraints when allowing them to be NOT VALID. > > > > > > This is simple and safe, since check constraints are not used in > > > planning until validated. > > > > The patch also reduces the lock level when NOT VALID is not specified, which didn't seem to be the intention. > > Thank you for reviewing. I agree that the behavior works as you indicated. > > My description of this was slightly muddled. The lock level for > CONSTR_FOREIGN applies whether or not NOT VALID is used, but the test > case covers only NOT VALID because it a) isn't tested and b) is more > important. I just followed that earlier pattern and that led me to > adding "NOT VALID" onto the title of the thread. > > What is true for CONSTR_FOREIGN is also true for CONSTR_CHECK - the > lock level can be set down to ShareRowExclusiveLock in all cases > because adding a new CHECK does not affect the outcome of currently > executing SELECT statements. (Note that this is not true for Drop > Constraint, which has a different lock level, but we aren't changing > that here). Once the constraint is validated it may influence the > optimization of later SELECTs. > > So the patch and included docs are completely correct. Notice that the > name of the patch reflects this better than the title of the thread. An additional patch covering other types of ALTER TABLE attached. Both can be applied independently. -- Simon Riggs http://www.EnterpriseDB.com/
Attachment
Simon Riggs <simon.riggs@enterprisedb.com> writes:
> 897795240cfaaed724af2f53ed2c50c9862f951f forgot to reduce the lock
> level for CHECK constraints when allowing them to be NOT VALID.
> This is simple and safe, since check constraints are not used in
> planning until validated.
Unfortunately, just asserting that it's safe doesn't make it so.
We have two things that we need to worry about when considering
reducing ALTER TABLE lock levels:
1. Is it semantically okay (which is what you claim above)?
2. Will onlooker processes see sufficiently-consistent catalog data
if they look at the table during the change?
Trying to reduce the lock level for ADD CHECK fails the second
test, because it has to alter two different catalogs. It has
to increment pg_class.relchecks, and it has to make an entry in
pg_constraint. This patch makes it possible for onlookers to
see a value of pg_class.relchecks that is inconsistent with what
they find in pg_constraint, and then they will blow up.
To demonstrate this, I applied the patch and then did this
in session 1:
regression=# create table mytable (f1 int check(f1 > 0), f2 int);
CREATE TABLE
I then started a second session, attached to it with gdb, and
set a breakpoint at CheckConstraintFetch. Letting that session
continue, I told it
regression=# select * from mytable;
which of course reached the breakpoint at CheckConstraintFetch.
(At this point, session 2 has read the pg_class entry for mytable,
seen relchecks == 1, and now it wants to read pg_constraint.)
I then told session 1:
regression=# alter table mytable add check (f2 > 0);
ALTER TABLE
which it happily did thanks to the now-inadequate lock level.
I then released session 2 to continue, and behold it complains:
WARNING: unexpected pg_constraint record found for relation "mytable"
LINE 1: select * from mytable;
^
(Pre-v14 branches would have made that an ERROR not a WARNING.)
That happens because the systable_beginscan() in CheckConstraintFetch
will get a new snapshot, so now it sees the new entry in pg_constraint,
making the count of entries inconsistent with what it found in pg_class.
It's possible that this could be made safe if we replaced the exact
"relchecks" count with a boolean "relhaschecks", analogous to the
way indexes are handled. It's not clear to me though that the effort,
and ensuing compatibility costs for applications that look at pg_class,
would be repaid by having a bit more concurrency here. One could
also worry about whether we really want to give up this consistency
cross-check between pg_class and pg_constraint.
regards, tom lane
On Sat, 4 Sept 2021 at 20:28, Tom Lane <tgl@sss.pgh.pa.us> wrote: > We have two things that we need to worry about when considering > reducing ALTER TABLE lock levels: > > 1. Is it semantically okay (which is what you claim above)? > > 2. Will onlooker processes see sufficiently-consistent catalog data > if they look at the table during the change? > > Trying to reduce the lock level for ADD CHECK fails the second > test, because it has to alter two different catalogs. It has > to increment pg_class.relchecks, and it has to make an entry in > pg_constraint. This patch makes it possible for onlookers to > see a value of pg_class.relchecks that is inconsistent with what > they find in pg_constraint, and then they will blow up. Thanks for the review. I will check this consideration for any future patches. > That happens because the systable_beginscan() in CheckConstraintFetch > will get a new snapshot, so now it sees the new entry in pg_constraint, > making the count of entries inconsistent with what it found in pg_class. This is clearly important and we must now return the patch with feedback. I've looked at other similar cases and can't find any bugs in other areas, phew! > It's possible that this could be made safe if we replaced the exact > "relchecks" count with a boolean "relhaschecks", analogous to the > way indexes are handled. It's not clear to me though that the effort, > and ensuing compatibility costs for applications that look at pg_class, > would be repaid by having a bit more concurrency here. One could > also worry about whether we really want to give up this consistency > cross-check between pg_class and pg_constraint. I will work on a patch for this and see how complex it is. At very least I will add a longer comment patch to mention this for the future. -- Simon Riggs http://www.EnterpriseDB.com/