> Heikki Linnakangas wrote:
> Setting the high bit in OldSetXidAdd() seems a bit weird. How about
> just using UINT64_MAX instead of 0 to mean no conflicts? Or 1, and
> start the sequence counter from 2.
Sure. I think I like reserving 1 and starting at 2 better. Will do.
> ReleasePredicateLocks() reads ShmemVariableCache->nextXid without
> XidGenLock. Maybe it's safe, we assume that TransactionIds are
> atomic elsewhere, but at least there needs to be a comment
> explaining it. But it probably should use ReadNewTransactionId()
> instead.
Hmm... We don't want to *assign* a new xid here -- we just want to
know what the next one *will be*. I will review locking around this
area, but I think it's sound. If that checks out OK on review, I'll
add comments.
> Attached is a patch for some trivial changes, mostly typos.
Wow. I transpose letters a lot don't I? Thanks for finding those.
Will fix.
> Overall, this is very neat and well-commented code. All the data
> structures make my head spin, but I don't see anything unnecessary
> or have any suggestions for simplification. There's a few remaining
> TODO comments in the code, which obviously need to be resolved one
> way or another, but as soon as you're finished with any outstanding
> issues that you know of, I think this is ready for commit.
OK. I may need to bounce some questions off the list to resolve some
of them. The biggest, in my mind, is whether MySerializableXact
needs to be declared volatile. I don't have my head around the
issues on that as well as I might. Any thoughts on it?
Thanks for the multiple reviews and all the suggestions. The code is
clearly better for your attention.
-Kevin