Reduce lock levels for ADD and DROP COLUMN
Idea is to reduce lock level of ADD/DROP COLUMN from AccessExclusiveLock
down to ShareRowExclusiveLock.
To make it work, we need to recognise that we are adding a column
without rewriting the table. That's a simple test at post parse analysis
stage, but what I can't do is work out whether the column name used is a
domain name which contains a constraint.
So if we want this, then it seems we need to add a separate subcommand,
so we can then throw an error if a domain is specified.
ALTER TABLE foo
ADD [COLUMN] colname CONCURRENTLY;
Or other ideas? Do we really care?
DROP ... RESTRICT works fine at reduced lock level, assuming I'm not
missing anything...
ALTER TABLE foo
DROP [COLUMN] colname RESTRICT;
Patch needs docs, tests and a check for the domain, so just a quick hack
just to get my dev muscles back in shape after Christmas. (Jokes
please).
Comments?
--
Simon Riggs http://www.2ndQuadrant.com/books/
PostgreSQL Development, 24x7 Support, Training and Services
Attachments:
alter_adddrop.v1.patchtext/x-patch; charset=UTF-8; name=alter_adddrop.v1.patchDownload
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 6729d83..96bd135 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -2512,7 +2512,6 @@ AlterTableGetLockLevel(List *cmds)
* New subcommand types should be added here by default.
*/
case AT_AddColumn: /* may rewrite heap, in some cases and visible to SELECT */
- case AT_DropColumn: /* change visible to SELECT */
case AT_AddColumnToView: /* CREATE VIEW */
case AT_AlterColumnType: /* must rewrite heap */
case AT_DropConstraint: /* as DROP INDEX */
@@ -2530,8 +2529,19 @@ AlterTableGetLockLevel(List *cmds)
break;
/*
+ * DROP can use a lower level of locking, if aren't using CASCADE
+ */
+ case AT_DropColumn: /* change visible to SELECT */
+ if (stmt->behavior == DROP_CASCADE)
+ cmd_lockmode = AccessExclusiveLock;
+ else
+ cmd_lockmode = ShareRowExclusiveLock;
+ break;
+
+ /*
* These subcommands affect write operations only.
*/
+ case AT_AddColumnNoQuals: /* must never rewrite heap */
case AT_ColumnDefault:
case AT_ProcessedConstraint: /* becomes AT_AddConstraint */
case AT_AddConstraintRecurse: /* becomes AT_AddConstraint */
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 8fc79b6..48a7b8c 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -1633,6 +1633,14 @@ alter_table_cmd:
n->def = $3;
$$ = (Node *)n;
}
+ /* ALTER TABLE <name> ADD COLUMN <coldef> */
+ | ADD_P COLUMN columnDefNoQuals CONCURRENTLY
+ {
+ AlterTableCmd *n = makeNode(AlterTableCmd);
+ n->subtype = AT_AddColumnNoQuals;
+ n->def = $3;
+ $$ = (Node *)n;
+ }
/* ALTER TABLE <name> ALTER [COLUMN] <colname> {SET DEFAULT <expr>|DROP DEFAULT} */
| ALTER opt_column ColId alter_column_default
{
@@ -2412,6 +2420,16 @@ columnDef: ColId Typename ColQualList
}
;
+columnDefNoQuals: ColId Typename
+ {
+ ColumnDef *n = makeNode(ColumnDef);
+ n->colname = $1;
+ n->typeName = $2;
+ n->is_local = true;
+ $$ = (Node *)n;
+ }
+ ;
+
columnOptions: ColId WITH OPTIONS ColQualList
{
ColumnDef *n = makeNode(ColumnDef);
On Mon, Dec 27, 2010 at 6:42 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
Idea is to reduce lock level of ADD/DROP COLUMN from AccessExclusiveLock
down to ShareRowExclusiveLock.To make it work, we need to recognise that we are adding a column
without rewriting the table.
Can you elaborate on why you think that's the right test? It seems to
me there could be code out there that assumes that the tuple
descriptor won't change under it while it holds an AccessShareLock.
What will happen if we're in the middle of returning tuples from a
large SELECT statement and we start seeing tuples with additional
attributes that we're not expecting? I'm particularly concerned about
cases where the user is doing "SELECT * FROM table" and the scan is
returning pointers to in-block tuples. If the schema suddenly changes
under us, we might need to start doing a projection step, but I think
the executor isn't going to know that.
If that's not a problem (how can we be confident of that?), then
perhaps ShareUpdateExclusive is just as good - if selects are OK, why
not inserts, updates, and deletes? There may be a reason, but I think
some analysis is needed here.
Incidentally, I notice that explicit-locking.html mentions that ALTER
TABLE may sometimes acquire AccessExclusiveLock and other times
ShareRowExclusiveLock, but it doesn't mention that it may sometimes
acquire only ShareUpdateExclusiveLock.
DROP ... RESTRICT works fine at reduced lock level, assuming I'm not
missing anything...
Same general issues here. Also, why is DROP .. RESTRICT different
from DROP .. CASCADE?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
On Mon, Dec 27, 2010 at 6:42 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
Idea is to reduce lock level of ADD/DROP COLUMN from AccessExclusiveLock
down to ShareRowExclusiveLock.To make it work, we need to recognise that we are adding a column
without rewriting the table.
Can you elaborate on why you think that's the right test? It seems to
me there could be code out there that assumes that the tuple
descriptor won't change under it while it holds an AccessShareLock.
s/could/definitely is/
I think this is guaranteed to break stuff; to the point that I'm
not even going to review the proposal in any detail.
regards, tom lane
On Mon, 2010-12-27 at 10:24 -0500, Robert Haas wrote:
On Mon, Dec 27, 2010 at 6:42 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
Idea is to reduce lock level of ADD/DROP COLUMN from AccessExclusiveLock
down to ShareRowExclusiveLock.To make it work, we need to recognise that we are adding a column
without rewriting the table.Can you elaborate on why you think that's the right test? It seems to
me there could be code out there that assumes that the tuple
descriptor won't change under it while it holds an AccessShareLock.
What will happen if we're in the middle of returning tuples from a
large SELECT statement and we start seeing tuples with additional
attributes that we're not expecting? I'm particularly concerned about
cases where the user is doing "SELECT * FROM table" and the scan is
returning pointers to in-block tuples. If the schema suddenly changes
under us, we might need to start doing a projection step, but I think
the executor isn't going to know that.
My understanding is that we access the columns directly by attid, so if
additional columns exist they would simply be ignored.
I fully expect this to be a difficult thing to believe, and I would say
there is a high potential that I am wrong about the ability to do this.
I have to say I was quite surprised when my prototype didn't immediately
explode. I'm posting before I've spent too long on the patch.
Feel free to shoot a hole in my continued optimism, anyone.
If that's not a problem (how can we be confident of that?), then
perhaps ShareUpdateExclusive is just as good - if selects are OK, why
not inserts, updates, and deletes? There may be a reason, but I think
some analysis is needed here.Incidentally, I notice that explicit-locking.html mentions that ALTER
TABLE may sometimes acquire AccessExclusiveLock and other times
ShareRowExclusiveLock, but it doesn't mention that it may sometimes
acquire only ShareUpdateExclusiveLock.
OK, will fix.
DROP ... RESTRICT works fine at reduced lock level, assuming I'm not
missing anything...Same general issues here. Also, why is DROP .. RESTRICT different
from DROP .. CASCADE?
The CASCADE causes other objects to be DROPped, which requires
AccessExclusiveLock. The RESTRICT case only works if nothing depends
upon that column. We already support fast drop, so the code already
allows people to ignore dropped columns they find when scanning.
--
Simon Riggs http://www.2ndQuadrant.com/books/
PostgreSQL Development, 24x7 Support, Training and Services
On Mon, 2010-12-27 at 10:41 -0500, Tom Lane wrote:
Robert Haas <robertmhaas@gmail.com> writes:
On Mon, Dec 27, 2010 at 6:42 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
Idea is to reduce lock level of ADD/DROP COLUMN from AccessExclusiveLock
down to ShareRowExclusiveLock.To make it work, we need to recognise that we are adding a column
without rewriting the table.Can you elaborate on why you think that's the right test? It seems to
me there could be code out there that assumes that the tuple
descriptor won't change under it while it holds an AccessShareLock.s/could/definitely is/
I think this is guaranteed to break stuff; to the point that I'm
not even going to review the proposal in any detail.
Our emails crossed.
Do you disagree with the ADD or the DROP, or both?
What "stuff" will break, in your opinion? I'm not asking you to do the
research, but a few curveballs would be enough to end this quickly, and
leave a good record for the archives.
--
Simon Riggs http://www.2ndQuadrant.com/books/
PostgreSQL Development, 24x7 Support, Training and Services
Simon Riggs <simon@2ndQuadrant.com> writes:
Do you disagree with the ADD or the DROP, or both?
Both.
What "stuff" will break, in your opinion? I'm not asking you to do the
research, but a few curveballs would be enough to end this quickly, and
leave a good record for the archives.
The most obvious point is that the parser, planner, and executor
all depend on the assumption that the lock originally taken in
parserOpenTable() is sufficient to ensure that the table definition
isn't moving underneath them. It will not be good if the executor
sees a different table definition than the parser saw.
To give just one concrete example of what's likely to go wrong, when we
read pg_attribute to construct the table's tuple descriptor, we do so
with SnapshotNow. If someone commits a change to one of those
pg_attribute rows while that scan is in progress, we may see both or
neither version of that pg_attribute row as valid.
I believe there are more subtle assumptions associated with the fact
that the relcache keeps a copy of the table's tupledesc; various places
assume that the cached tupdesc won't change or disappear underneath them
as long as they've got some lock on the table.
regards, tom lane
On Mon, 2010-12-27 at 13:33 -0500, Tom Lane wrote:
Simon Riggs <simon@2ndQuadrant.com> writes:
Do you disagree with the ADD or the DROP, or both?
Both.
What "stuff" will break, in your opinion? I'm not asking you to do the
research, but a few curveballs would be enough to end this quickly, and
leave a good record for the archives.The most obvious point is that the parser, planner, and executor
all depend on the assumption that the lock originally taken in
parserOpenTable() is sufficient to ensure that the table definition
isn't moving underneath them. It will not be good if the executor
sees a different table definition than the parser saw.
To give just one concrete example of what's likely to go wrong, when we
read pg_attribute to construct the table's tuple descriptor, we do so
with SnapshotNow. If someone commits a change to one of those
pg_attribute rows while that scan is in progress, we may see both or
neither version of that pg_attribute row as valid.
That sounds like a big problem.
I believe there are more subtle assumptions associated with the fact
that the relcache keeps a copy of the table's tupledesc; various places
assume that the cached tupdesc won't change or disappear underneath them
as long as they've got some lock on the table.
Thanks for explaining.
At this stage of 9.1, I will immediately stop pursuing this possibility.
This is an important area for us for the future, since crowds gather
over RDBMS' collective inability to add/drop new attributes easily.
--
Simon Riggs http://www.2ndQuadrant.com/books/
PostgreSQL Development, 24x7 Support, Training and Services