Thread: 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
Attachment
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