Re: 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(at)csail(dot)mit(dot)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(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre(at)commandprompt(dot)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(dot)linnakangas(at)enterprisedb(dot)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

On Wed, 2011-06-08 at 17:29 -0500, Kevin Grittner wrote:
> Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)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(at)j-davis(dot)com> writes:
> On Wed, 2011-06-08 at 17:29 -0500, Kevin Grittner wrote:
>> Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)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