Thread: SSI heap_insert and page-level predicate locks
heap_insert() calls CheckForSerializableConflictIn(), which checks if there is a predicate lock on the whole relation, or on the page we're inserting to. It does not check for tuple-level locks, because there can't be any locks on a tuple that didn't exist before. AFAICS, the check for page lock is actually unnecessary. A page-level lock on a heap only occurs when tuple-level locks are promoted. It is just a coarser-grain representation of holding locks on all tuples on the page, *that exist already*. It is not a "gap" lock like the index locks are, it doesn't need to conflict with inserting new tuples on the page. In fact, if heap_insert chose to insert the tuple on some other heap page, there would have been no conflict. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
On Wed, Jun 08, 2011 at 11:23:48AM +0300, Heikki Linnakangas wrote: > AFAICS, the check for page lock is actually unnecessary. A page-level > lock on a heap only occurs when tuple-level locks are promoted. It is > just a coarser-grain representation of holding locks on all tuples on > the page, *that exist already*. It is not a "gap" lock like the index > locks are, it doesn't need to conflict with inserting new tuples on the > page. In fact, if heap_insert chose to insert the tuple on some other > heap page, there would have been no conflict. Yes, it's certainly unnecessary now, given that we never explicitly take heap page locks, just tuple- or relation-level. The only thing I'd be worried about is that at some future point we might add heap page locks -- say, for sequential scans that don't read the entire relation -- and expect inserts to be tested against them. I'm not sure whether we'd actually do this, but we wanted to keep the option open during development. Dan -- Dan R. K. Ports MIT CSAIL http://drkp.net/
On 08.06.2011 12:36, Dan Ports wrote: > On Wed, Jun 08, 2011 at 11:23:48AM +0300, Heikki Linnakangas wrote: >> AFAICS, the check for page lock is actually unnecessary. A page-level >> lock on a heap only occurs when tuple-level locks are promoted. It is >> just a coarser-grain representation of holding locks on all tuples on >> the page, *that exist already*. It is not a "gap" lock like the index >> locks are, it doesn't need to conflict with inserting new tuples on the >> page. In fact, if heap_insert chose to insert the tuple on some other >> heap page, there would have been no conflict. > > Yes, it's certainly unnecessary now, given that we never explicitly > take heap page locks, just tuple- or relation-level. > > The only thing I'd be worried about is that at some future point we > might add heap page locks -- say, for sequential scans that don't read > the entire relation -- and expect inserts to be tested against them. > I'm not sure whether we'd actually do this, but we wanted to keep the > option open during development. I think that is only relevant for queries like "SELECT * FROM table WHERE ctid = '(12,34)'. You might expect that we take a lock on that physical part of the heap, so that an INSERT that falls on that slot would conflict, but one that falls elsewhere does not. At the moment, a TidScan only takes a predicate lock tuples that exist, it doesn't try to lock the range like an IndexScan would. The physical layout of the table is an implementation detail that the application shouldn't care about, so I don't feel sorry for anyone doing that. Maybe it's worth mentioning somewhere in the docs, though. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
On Wed, Jun 8, 2011 at 5:36 AM, Dan Ports <drkp@csail.mit.edu> wrote: > On Wed, Jun 08, 2011 at 11:23:48AM +0300, Heikki Linnakangas wrote: >> AFAICS, the check for page lock is actually unnecessary. A page-level >> lock on a heap only occurs when tuple-level locks are promoted. It is >> just a coarser-grain representation of holding locks on all tuples on >> the page, *that exist already*. It is not a "gap" lock like the index >> locks are, it doesn't need to conflict with inserting new tuples on the >> page. In fact, if heap_insert chose to insert the tuple on some other >> heap page, there would have been no conflict. > > Yes, it's certainly unnecessary now, given that we never explicitly > take heap page locks, just tuple- or relation-level. > > The only thing I'd be worried about is that at some future point we > might add heap page locks -- say, for sequential scans that don't read > the entire relation -- and expect inserts to be tested against them. > I'm not sure whether we'd actually do this, but we wanted to keep the > option open during development. I don't think this is the right time to be rejiggering this stuff anyway. Our bug count is -- at least to outward appearances -- remarkably low at the moment, considering how much stuff we jammed into this release. Let's not hastily change things we might later regret having changed. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Excerpts from Heikki Linnakangas's message of mié jun 08 05:45:35 -0400 2011: > On 08.06.2011 12:36, Dan Ports wrote: > > The only thing I'd be worried about is that at some future point we > > might add heap page locks -- say, for sequential scans that don't read > > the entire relation -- and expect inserts to be tested against them. > > I'm not sure whether we'd actually do this, but we wanted to keep the > > option open during development. > > I think that is only relevant for queries like "SELECT * FROM table > WHERE ctid = '(12,34)'. You might expect that we take a lock on that > physical part of the heap, so that an INSERT that falls on that slot > would conflict, but one that falls elsewhere does not. At the moment, a > TidScan only takes a predicate lock tuples that exist, it doesn't try to > lock the range like an IndexScan would. > > The physical layout of the table is an implementation detail that the > application shouldn't care about, so I don't feel sorry for anyone doing > that. Maybe it's worth mentioning somewhere in the docs, though. What about UPDATE WHERE CURRENT OF? Also, people sometimes use CTID to eliminate duplicate rows. -- Álvaro Herrera <alvherre@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera <alvherre@commandprompt.com> wrote: > Excerpts from Heikki Linnakangas's message of mié jun 08 05:45:35 -0400 2011: >> On 08.06.2011 12:36, Dan Ports wrote: > >>> The only thing I'd be worried about is that at some future point >>> we might add heap page locks -- say, for sequential scans that >>> don't read the entire relation -- and expect inserts to be >>> tested against them. I'm not sure whether we'd actually do >>> this, but we wanted to keep the option open during development. >> >> I think that is only relevant for queries like "SELECT * FROM >> table WHERE ctid = '(12,34)'. You might expect that we take a >> lock on that physical part of the heap, so that an INSERT that >> falls on that slot would conflict, but one that falls elsewhere >> does not. At the moment, a TidScan only takes a predicate lock >> tuples that exist, it doesn't try to lock the range like an >> IndexScan would. >> >> The physical layout of the table is an implementation detail that >> the application shouldn't care about, so I don't feel sorry for >> anyone doing that. Maybe it's worth mentioning somewhere in the >> docs, though. Agreed. I'll add it to my list. > What about UPDATE WHERE CURRENT OF? That doesn't currently lock anything except rows actually read, does it? If not, that has no bearing on the suggested change. Also, any row read through any *other* means during the same transaction would already have a predicate lock which would cover the tuple, so any case where you read the TID from a tuple and then use that to re-visit the tuple during the same transaction would not be affected. We're talking about whether it makes any sense to blindly read a TID, and if it's not found, to remember the fact that that particular TID *wasn't* there, and generate a rw-conflict if an overlapping transaction does something which *creates* a tuple with that TID. That does seem to be an unlikely use case. If we decide not to support SSI conflict detection in that usage, we can save some CPU time, reduce LW lock contention, and reduce the false positive rate for serialization failures. > Also, people sometimes use CTID to eliminate duplicate rows. Again, I presume that if they want transactional behavior on that, they would read the duplicates and do the delete within the same transaction? If so there's no chance of a problem. If not, we're talking about wanting to create a rw-conflict with an overlapping transaction which reused the same TID, which I'm not sure is even possible, or that it makes sense to care about the TID matching anyway. -Kevin
Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> wrote: > heap_insert() calls CheckForSerializableConflictIn(), which checks if > there is a predicate lock on the whole relation, or on the page we're > inserting to. It does not check for tuple-level locks, because there > can't be any locks on a tuple that didn't exist before. > > AFAICS, the check for page lock is actually unnecessary. A page-level > lock on a heap only occurs when tuple-level locks are promoted. It is > just a coarser-grain representation of holding locks on all tuples on > the page, *that exist already*. It is not a "gap" lock like the index > locks are, it doesn't need to conflict with inserting new tuples on the > page. In fact, if heap_insert chose to insert the tuple on some other > heap page, there would have been no conflict. Absolutely correct. Patch attached. -Kevin
Attachment
On Wed, 2011-06-08 at 17:29 -0500, Kevin Grittner wrote: > Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> wrote: > > heap_insert() calls CheckForSerializableConflictIn(), which checks if > > there is a predicate lock on the whole relation, or on the page we're > > inserting to. It does not check for tuple-level locks, because there > > can't be any locks on a tuple that didn't exist before. > > AFAICS, the check for page lock is actually unnecessary. A page-level > > lock on a heap only occurs when tuple-level locks are promoted. It is > > just a coarser-grain representation of holding locks on all tuples on > > the page, *that exist already*. It is not a "gap" lock like the index > > locks are, it doesn't need to conflict with inserting new tuples on > the > > page. In fact, if heap_insert chose to insert the tuple on some other > > heap page, there would have been no conflict. > > Absolutely correct. Patch attached. I like the change, but the comment is slightly confusing. Perhaps something like: "For a heap insert, we only need to check for table locks. Our new tuple can't possibly conflict with existing tuple locks, and heap page locks are only consolidated versions of tuple locks. The index insert will check for any other predicate locks." would be a little more clear? (the last sentence is optional, and I only included it because the original comment mentioned indexes). Same for heap_update(). Regards,Jeff Davis
Jeff Davis <pgsql@j-davis.com> writes:
> On Wed, 2011-06-08 at 17:29 -0500, Kevin Grittner wrote:
>> Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> wrote:
>>> AFAICS, the check for page lock is actually unnecessary.
>> Absolutely correct. Patch attached.
> I like the change, but the comment is slightly confusing.
I've committed this patch with comment rewording along the lines
suggested by Jeff. I also moved the CheckForSerializableConflictIn call
to just before, instead of just after, the RelationGetBufferForTuple
call. We no longer have to do it after, since we don't need to know
which buffer to pass, and it should buy some more low-level parallelism
to run the SSI checks while not holding exclusive lock on the eventual
target buffer.
regards, tom lane