Review: Row-level Locks & SERIALIZABLE transactions, postgres vs. Oracle
in [PgSql]
|
Prev: Review: Patch for phypot - Pygmy Hippotause
Next: Review: Row-level Locks & SERIALIZABLE transactions,postgres vs. Oracle
From: "Kevin Grittner" on 17 Jul 2010 12:25 ================= 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. -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers |