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/
Attachments:
alter_table_add_check_constraint_reduce_lock_level.v1.patchapplication/octet-stream; name=alter_table_add_check_constraint_reduce_lock_level.v1.patchDownload
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 39927be41e..53ecda3d2e 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -429,7 +429,8 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
Although most forms of <literal>ADD
<replaceable class="parameter">table_constraint</replaceable></literal>
require an <literal>ACCESS EXCLUSIVE</literal> lock, <literal>ADD
- FOREIGN KEY</literal> requires only a <literal>SHARE ROW
+ CHECK</literal> and <literal>ADD
+ FOREIGN KEY</literal> require only a <literal>SHARE ROW
EXCLUSIVE</literal> lock. Note that <literal>ADD FOREIGN KEY</literal>
also acquires a <literal>SHARE ROW EXCLUSIVE</literal> lock on the
referenced table, in addition to the lock on the table on which the
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 97cc9fd6ec..6e0ff5c08f 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -4233,6 +4233,15 @@ AlterTableGetLockLevel(List *cmds)
*/
cmd_lockmode = AccessExclusiveLock;
break;
+
+ case CONSTR_CHECK:
+
+ /*
+ * We prevent other writers from accessing table.
+ */
+ cmd_lockmode = ShareRowExclusiveLock;
+ break;
+
case CONSTR_FOREIGN:
/*
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index f81bdf513b..8bf6e284bb 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -2729,6 +2729,15 @@ select * from my_locks order by 1;
alterlock_pkey | AccessShareLock
(4 rows)
+rollback;
+begin;
+alter table alterlock2 add check (f1 > 0) not valid;
+select * from my_locks order by 1;
+ relname | max_lockmode
+------------+-----------------------
+ alterlock2 | ShareRowExclusiveLock
+(1 row)
+
rollback;
begin;
alter table alterlock2
diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index dc0200adcb..a65c9a7465 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -1744,6 +1744,11 @@ alter table alterlock2 add foreign key (f1) references alterlock (f1);
select * from my_locks order by 1;
rollback;
+begin;
+alter table alterlock2 add check (f1 > 0) not valid;
+select * from my_locks order by 1;
+rollback;
+
begin;
alter table alterlock2
add constraint alterlock2nv foreign key (f1) references alterlock (f1) NOT VALID;
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
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/
Attachments:
alter_table_reduce_more_lock_levels.v1.patchapplication/octet-stream; name=alter_table_reduce_more_lock_levels.v1.patchDownload
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 03dfd2e7fa..534b72c36f 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -4161,29 +4161,28 @@ AlterTableGetLockLevel(List *cmds)
case AT_DisableTrig:
case AT_DisableTrigAll:
case AT_DisableTrigUser:
+ case AT_ColumnDefault:
+ case AT_CookedColumnDefault:
+ case AT_ReplicaIdentity:
+ case AT_AlterConstraint:
+ case AT_AddIdentity:
+ case AT_DropIdentity:
+ case AT_SetIdentity:
+ case AT_DropExpression:
+ case AT_SetCompression:
cmd_lockmode = ShareRowExclusiveLock;
break;
/*
- * These subcommands affect write operations only. XXX
- * Theoretically, these could be ShareRowExclusiveLock.
+ * These subcommands appear to have various special cases.
*/
- case AT_ColumnDefault:
- case AT_CookedColumnDefault:
- case AT_AlterConstraint:
case AT_AddIndex: /* from ADD CONSTRAINT */
case AT_AddIndexConstraint:
- case AT_ReplicaIdentity:
case AT_SetNotNull:
case AT_EnableRowSecurity:
case AT_DisableRowSecurity:
case AT_ForceRowSecurity:
case AT_NoForceRowSecurity:
- case AT_AddIdentity:
- case AT_DropIdentity:
- case AT_SetIdentity:
- case AT_DropExpression:
- case AT_SetCompression:
cmd_lockmode = AccessExclusiveLock;
break;
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 8dcb00ac67..d35a7c9b51 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -2697,9 +2697,9 @@ select * from my_locks order by 1;
rollback;
begin; alter table alterlock alter column f2 set default 'x';
select * from my_locks order by 1;
- relname | max_lockmode
------------+---------------------
- alterlock | AccessExclusiveLock
+ relname | max_lockmode
+-----------+-----------------------
+ alterlock | ShareRowExclusiveLock
(1 row)
rollback;
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/