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

On Jul17, 2010, at 18:25 , Kevin Grittner wrote:
> * Does it include reasonable tests, necessary doc patches, etc?
>
> Documentation changes are needed in the "Concurrency Control"
> chapter.
>
> <...>
>
> * Do we want that?
>
> Yes. We seem to have reached consensus on the -hackers list to that
> effect.
I've kinda waited for the latter before putting work into the former, so I guess I should get going.

> * Does the patch slow down simple tests?
The part that I'm slightly nervous about regarding performance regressions is the call of MultiXactIdSetOldestVisible() I had to add to GetTransactionSnapshot() (Though only for serializable transactions). That could increase the pressure on the multi xact machinery since for some workloads since it will increase the time a multi xact stays alive. I don't see a way around that, though, since the patch depends on being able to find multi xact members which are invisible to a serializable transaction's snapshot.

> * Does it follow the project coding guidelines?
>
> Comments are not all in standard style.
Does that refer to the language used, or to the formatting?

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

> 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.
Yeah, that was the idea.

Btw, while the patch obsoletes the crosscheck snapshot, it currently doesn't remove its traces of it throughout the executor and the ri triggers. Mainly because I felt doing so would make forward-porting and reviewing harder without any gain. But ultimately, those traces should probably all go, unless someone feels that for some #ifdef NOT_USED is preferable.

> * 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?
Hm, I'll try to come up with something.

> * Are there interdependencies that can cause problems?
>
> 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.

Cool! And thanks for the work you put into reviewing this! I'll update the documentation and comments ASAP. Probably not within the next few days though, since I'm unfortunately quite busy at the moment, trying to finally complete my master's thesis.

best regards,
Florian Pflug