Thread

Reduce lock levels for ADD and DROP COLUMN

Simon Riggs <simon@2ndquadrant.com> — 2010-12-27T11:42:34Z

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

Re: Reduce lock levels for ADD and DROP COLUMN

Robert Haas <robertmhaas@gmail.com> — 2010-12-27T15:24:20Z

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

Re: Reduce lock levels for ADD and DROP COLUMN

Tom Lane <tgl@sss.pgh.pa.us> — 2010-12-27T15:41:55Z

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

Re: Reduce lock levels for ADD and DROP COLUMN

Simon Riggs <simon@2ndquadrant.com> — 2010-12-27T15:52:47Z

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

Re: Reduce lock levels for ADD and DROP COLUMN

Simon Riggs <simon@2ndquadrant.com> — 2010-12-27T16:04:40Z

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

Re: Reduce lock levels for ADD and DROP COLUMN

Tom Lane <tgl@sss.pgh.pa.us> — 2010-12-27T18:33:02Z

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

Re: Reduce lock levels for ADD and DROP COLUMN

Simon Riggs <simon@2ndquadrant.com> — 2010-12-27T19:51:43Z

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