Re: Review: Row-level Locks & SERIALIZABLE transactions, postgres vs. Oracle

=================
Submission review
=================

* Is the patch in context diff format?

Yes.

* Does it apply cleanly to the current CVS HEAD?

Yes.

* Does it include reasonable tests, necessary doc patches, etc?

There is one pgbench test which shows incorrect behavior without the
patch and correct behavior with the patch for a significant use
case.

Documentation changes are needed in the "Concurrency Control"
chapter.


================
Usability review
================

* Does the patch actually implement that?

Yes.

* Do we want that?

Yes. We seem to have reached consensus on the -hackers list to that
effect. On a personal note, I heard some current Oracle users who
were considering a switch to PostgreSQL grumbling after Robert's
trigger presentation at PostgreSQL Conference U.S. East about how
they didn't need to use such complex coding techniques to ensure
data integrity in Oracle as is required in PostgreSQL. I was
surprised, since I know that they also get snapshot isolation when
they request serializable, but they explained that SELECT FOR UPDATE
provides stronger guarantees in Oracle. This patch should provide
equivalent behavior, which should ease migration from Oracle and
allow simpler coding techniques in snapshot isolation to protect
data.

* Do we already have it?

No.

* Does it follow SQL spec, or the community-agreed behavior?

It's not in the spec, but it moves to safer behavior which is
consistent with the current Oracle implementation.

* Does it include pg_dump support (if applicable)?

Not applicable.

* Are there dangers?

Some code which continues after blocking will now get a
serialization failure. It's possible that this could cause problems
for some existing code, although that code was likely either using
SELECT FOR UPDATE unnecessarily or was unsafe without this patch.

* Have all the bases been covered?

As far as I can see.


============
Feature test
============

* Does the feature work as advertised?

Yes.

* Are there corner cases the author has failed to consider?

Not that I found.

* Are there any assertion failures or crashes?

No.


==================
Performance review
==================

* Does the patch slow down simple tests?

No.

* If it claims to improve performance, does it?

It makes no such claim.

* Does it slow down other things?

No.


=============
Coding review
=============

* Does it follow the project coding guidelines?

Comments are not all in standard style.

In some cases there are unnecessary braces around a single statement
for an *if* or *else*.

There are function where the last two parameters were changed from:

Snapshot crosscheck, bool wait

to:

bool wait, Snapshot lockcheck_snapshot

It appears to be so that the semantic change to the use of the
snapshot doesn't break code at runtime without forcing the
programmer to notice the change based on compile errors, which seems
like a Good Thing.

* Are there portability issues?

No.

* Will it work on Windows/BSD etc?

Yes.

* Are the comments sufficient and accurate?

Given that there is a significant behavioral change, it seems as
though there could be a sentence or two somewhere concisely stating
the how things behave, but I'm not sure quite where it would go.
Perhaps something in the README file in the access/transam
directory?

* Does it do what it says, correctly?

Yes.

* Does it produce compiler warnings?

No.

* Can you make it crash?

No.


===================
Architecture review
===================

* Is everything done in a way that fits together coherently with
other features/modules?

I think so.

* Are there interdependencies that can cause problems?

Not that I could identify with committed code.

I was concerned about its interaction with the other serializable
patch (by myself and Dan Ports), so I also combined the patches and
tested. Florian's pgbench test did expose bugs in the *other*
patch, which I then fixed in the combined setting. There was still
some breakage in the other patch when Florian's patch was backed
out, so at the moment, this patch would appear to be a
*prerequisite* for the other. (That can hopefully be corrected
by changes to the other patch.)

Also, I'm attempting to adapt the dcheck tests for the other patch
to work with this patch. If successful, I'll post with the results
of that additional testing.