Re: SSI work for 9.1

Dan Ports wrote:
> On Wed, Jun 08, 2011 at 09:17:04PM -0500, Kevin Grittner wrote:

>> A patch is attached which just covers the predicate lock
>> acquisition, where a snapshot is available without too much pain.
>> There are two functions which acquire predicate locks where a
>> snapshot was not readily available: _bt_search() and
>> _bt_get_endpoint(). Not only was it not clear how to get a
>> snapshot in, it was not entirely clear from reading the code that
>> we need to acquire predicate locks here. Now, I suspect that we
>> probably do, because I spent many long hours stepping through gdb
>> to pick the spots where they are, but that was about a year ago
>> and my memory of the details has faded.
>
> For _bt_search(), the lock calls should move to _bt_first() where
> the ScanDesc is available. This also keeps us from trying to take
> locks during _bt_pagedel(), which is only called during vacuum and
> recovery.

Sounds reasonable, but why did you pass the snapshot to the
PredicateLockPage() call but not the PredicateLockRelation() call?
Oversight?

> The call in _bt_get_endpoint() seems unnecessary, because after it
> returns, _bt_endpoint() takes the same lock. The only other callers
> of _bt_get_endpoint() are _bt_pagedel() and _bt_insert_parent(),
> neither of which should take predicate locks.

That also sounds reasonable.

> I've updated the patch, attached.

I've confirmed that it passes the usual regression tests (including
isolation tests and the normal regression tests at serializable).
I'll take a closer look once I wake up and get the caffeine going.

Thanks for following up on this!

-Kevin

On Thu, Jun 09, 2011 at 07:06:18AM -0500, Kevin Grittner wrote:
> Sounds reasonable, but why did you pass the snapshot to the
> PredicateLockPage() call but not the PredicateLockRelation() call?
> Oversight?

Yep, just an oversight; long day yesterday. I'll fix the patch shortly
(unless you can get to it first, in which case I wouldn't complain)

Dan

--
Dan R. K. Ports MIT CSAIL http://drkp.net/

On Thu, Jun 09, 2011 at 01:30:27PM -0400, Dan Ports wrote:
> On Thu, Jun 09, 2011 at 07:06:18AM -0500, Kevin Grittner wrote:
> > Sounds reasonable, but why did you pass the snapshot to the
> > PredicateLockPage() call but not the PredicateLockRelation() call?
> > Oversight?
>
> Yep, just an oversight; long day yesterday. I'll fix the patch shortly

Attached. Only change is the missing snapshot argument to that one
call.

Dan

--
Dan R. K. Ports MIT CSAIL http://drkp.net/

Dan Ports <drkp(at)csail(dot)mit(dot)edu> wrote:
> On Thu, Jun 09, 2011 at 01:30:27PM -0400, Dan Ports wrote:
>> On Thu, Jun 09, 2011 at 07:06:18AM -0500, Kevin Grittner wrote:
>>> Sounds reasonable, but why did you pass the snapshot to the
>>> PredicateLockPage() call but not the PredicateLockRelation()
>>> call? Oversight?
>>
>> Yep, just an oversight; long day yesterday. I'll fix the patch
>> shortly
>
> Attached. Only change is the missing snapshot argument to that one
> call.

I am in full agreement with this patch.

It's an interesting coincidence that the two predicate locking calls
which were at the wrong level to have access to the snapshot
information were also at the wrong level to be able to fire them at
only the needed times.

-Kevin

"Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> wrote:

> I am in full agreement with this patch.

I found that pgindent would like to tweak whitespace in three places
in that patch, and I found an unnecessary include that I would like
to remove. Normally, I would post a new version of the patch with
those adjustments, but there's been a disquieting tendency for
people to not look at what's actually happening with revisions of
this patch and act like the sky is falling with each refinement.

Let me be clear: each posted version of this patch has been safe and
has been an improvement on what came before. Dan and I didn't
disagree about what to do at any point; Dan figured out what to do
with two calls I left alone because I faded before I could work out
how to deal with them. Essentially we collaborated on-list rather
than discussing things off-list and posting the end result. Perhaps
that was a bad choice, but time was short and I had hopes that a
change people had requested could be included in beta2.

Here are the tweaks which should be applied after Dan's last version
of the patch. If people prefer, I'll post a roll-up including them.

http://git.postgresql.org/gitweb?p=users/kgrittn/postgres.git;a=commitdiff;h=0258af4168a130af0d1e52294b248d54715b5f72

http://git.postgresql.org/gitweb?p=users/kgrittn/postgres.git;a=commitdiff;h=bb951bacd9700cdbddb0beba338a63bd301b200d

-Kevin

On 10.06.2011 19:05, Kevin Grittner wrote:
> I found that pgindent would like to tweak whitespace in three places
> in that patch, and I found an unnecessary include that I would like
> to remove. Normally, I would post a new version of the patch with
> those adjustments, but there's been a disquieting tendency for
> people to not look at what's actually happening with revisions of
> this patch and act like the sky is falling with each refinement.
>
> Let me be clear: each posted version of this patch has been safe and
> has been an improvement on what came before. Dan and I didn't
> disagree about what to do at any point; Dan figured out what to do
> with two calls I left alone because I faded before I could work out
> how to deal with them. Essentially we collaborated on-list rather
> than discussing things off-list and posting the end result. Perhaps
> that was a bad choice, but time was short and I had hopes that a
> change people had requested could be included in beta2.

I did some further changes, refactoring SkipSerialization so that it's
hopefully more readable, and added a comment about the side-effects. See
attached. Let me know if I'm missing something.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:

> I did some further changes, refactoring SkipSerialization so that
> it's hopefully more readable, and added a comment about the
> side-effects. See attached. Let me know if I'm missing something.

I do think the changes improve readability. I don't see anything
missing, but there's something we can drop. Now that you've split
the read and write tests, this part can be dropped from the
SerializationNeededForWrite function:

+
+ /* Check if we have just become "RO-safe". */
+ if (SxactIsROSafe(MySerializableXact))
+ {
+ ReleasePredicateLocks(false);
+ return false;
+ }

If it's doing a write, it can't be a read-only transaction....

-Kevin

On 14.06.2011 17:57, Kevin Grittner wrote:
> Heikki Linnakangas<heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
>
>> I did some further changes, refactoring SkipSerialization so that
>> it's hopefully more readable, and added a comment about the
>> side-effects. See attached. Let me know if I'm missing something.
>
> I do think the changes improve readability. I don't see anything
> missing, but there's something we can drop. Now that you've split
> the read and write tests, this part can be dropped from the
> SerializationNeededForWrite function:
>
> +
> + /* Check if we have just become "RO-safe". */
> + if (SxactIsROSafe(MySerializableXact))
> + {
> + ReleasePredicateLocks(false);
> + return false;
> + }
>
> If it's doing a write, it can't be a read-only transaction....

Ah, good point. Committed with that removed.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

On Wed, Jun 15, 2011 at 5:10 AM, Heikki Linnakangas
<heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
> On 14.06.2011 17:57, Kevin Grittner wrote:
>>
>> Heikki Linnakangas<heikki(dot)linnakangas(at)enterprisedb(dot)com>  wrote:
>>
>>> I did some further changes, refactoring SkipSerialization so that
>>> it's hopefully more readable, and added a comment about the
>>> side-effects. See attached. Let me know if I'm missing something.
>>
>> I do think the changes improve readability.  I don't see anything
>> missing, but there's something we can drop.  Now that you've split
>> the read and write tests, this part can be dropped from the
>> SerializationNeededForWrite function:
>>
>> +
>> +       /* Check if we have just become "RO-safe". */
>> +       if (SxactIsROSafe(MySerializableXact))
>> +       {
>> +               ReleasePredicateLocks(false);
>> +               return false;
>> +       }
>>
>> If it's doing a write, it can't be a read-only transaction....
>
> Ah, good point. Committed with that removed.

Does this mean that the open item "more SSI loose ends" can now be
marked resolved?

http://wiki.postgresql.org/wiki/PostgreSQL_9.1_Open_Items

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Jun 16, 2011 at 11:49:48PM -0400, Robert Haas wrote:
> Does this mean that the open item "more SSI loose ends" can now be
> marked resolved?

I was just looking at it and contemplating moving it to the non-blockers
list. Of the five items:
- (1) and (4) are resolved
- (2) isn't an issue -- maybe we want to add a comment, someplace but
I'm not convinced even that is necessary
- (3) is a regression test, and is already on the list separately
- (5) is a doc issue only

There are no open issues with the code that I'm aware of.

Dan

--
Dan R. K. Ports MIT CSAIL http://drkp.net/

On Fri, Jun 17, 2011 at 12:30 AM, Dan Ports <drkp(at)csail(dot)mit(dot)edu> wrote:
> On Thu, Jun 16, 2011 at 11:49:48PM -0400, Robert Haas wrote:
>> Does this mean that the open item "more SSI loose ends" can now be
>> marked resolved?
>
> I was just looking at it and contemplating moving it to the non-blockers
> list. Of the five items:
>  - (1) and (4) are resolved
>  - (2) isn't an issue -- maybe we want to add a comment, someplace but
>   I'm not convinced even that is necessary
>  - (3) is a regression test, and is already on the list separately
>  - (5) is a doc issue only
>
> There are no open issues with the code that I'm aware of.

Perhaps it would be best to remove the general item and replace it
with a list of more specific things that need doing - which might just
mean #5.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, Jun 17, 2011 at 12:32:46AM -0400, Robert Haas wrote:
> Perhaps it would be best to remove the general item and replace it
> with a list of more specific things that need doing - which might just
> mean #5.

Done.

--
Dan R. K. Ports MIT CSAIL http://drkp.net/