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
Attachments:
ssi-heap-insert-fix-1.patchtext/plain; name=ssi-heap-insert-fix-1.patchDownload
*** a/src/backend/access/heap/heapam.c
--- b/src/backend/access/heap/heapam.c
***************
*** 1916,1926 **** heap_insert(Relation relation, HeapTuple tup, CommandId cid,
InvalidBuffer, options, bistate);
/*
! * We're about to do the actual insert -- check for conflict at the
! * relation or buffer level first, to avoid possibly having to roll back
! * work we've just done.
*/
! CheckForSerializableConflictIn(relation, NULL, buffer);
/* NO EREPORT(ERROR) from here till changes are logged */
START_CRIT_SECTION();
--- 1916,1929 ----
InvalidBuffer, options, bistate);
/*
! * We're about to do the actual insert -- check for conflict first, to
! * avoid possibly having to roll back work we've just done.
! *
! * NOTE: For a tuple insert, we only need to check for table locks, since
! * predicate locking at the index level will cover ranges for anything
! * except a table scan. Therefore, only provide the relation.
*/
! CheckForSerializableConflictIn(relation, NULL, InvalidBuffer);
/* NO EREPORT(ERROR) from here till changes are logged */
START_CRIT_SECTION();
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 onthe
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