Re: [PATCH] Lazy xid assingment V2

Tom Lane wrote:
> "Florian G. Pflug" <fgp(at)phlo(dot)org> writes:
> A few comments ...
>
>> GetOldestXmin() is changed slightly - it used to use
>> GetTopTransactionId() as a starting point for it's xmin
>> calculation, falling back to ReadNewTransactionId() if called
>> from outside a transaction. Now, it always starts with ReadNewTransactionId().
>
> It's kind of annoying to do ReadNewTransactionId() when most of the time
> it won't affect the result. However I don't see much choice; if we
> tried to read it only after failing to find any entries in the
> procarray, we'd have a race condition, since someone might acquire
> an xid and put it into the procarray after we look at their PGPROC
> and before we can do ReadNewTransactionId.
>
> It might be worth doing GetTopTransactionIdIfAny and only calling
> ReadNewTransactionId when that fails, since the former is far
> cheaper than the latter. Also we could cheaply check to see if
> our own xmin is set, and use that to initialize the computation.
Sounds worthwile, I'm going to do this.

>> The function xid_age computes the age of a given xid relative
>> to the transaction's toplevel xid. Since forcing xid assignment
>> just to do that computation seemed silly, I modified xid_age
>> to use MyProc->xmin as a reference instead. I could have used
>> ReadNewTransactionId(), but that involves locking overhead,
>> and we probably want to use something that remains constant
>> during a transaction.
>
> I don't much like this, since as I mentioned before I don't think
> MyProc->xmin is going to be constant over a whole transaction for
> long. I don't think xid_age is performance-critical in any way,
> so I'd vote for letting it force XID assignment.
Hm... I agree that xid_age is probably not performance-critical.
I guess it's more the complete counter-intuitivity of forcing
xid assignment in some arbitrary function thats bugging me a bit.

Maybe it'd be sufficient to guarantee a constant return value
of this function within one statement for read-committed
transaction? For serializable transactions I'd still be constant
Sounds not completely ill-logical to me - after all the age
of an xid *does* really change if my xmin moves forward.

But I can live with just doing GetTopTransactionId() too...

>> The first requirement is already satisfied - file creation
>> will surely be followed by a catalog update, which will force xid assingment,
>> and thus writing a COMMIT record. There is an assertion in
>> RecordTransactionCommit that checks for no schedules file deletions when
>> we don't have a xid assigned.
>
> Rather than an Assert (which won't be there in production), I'd suggest
> a standard test and elog(ERROR). This point is sufficiently critical
> that I don't want the test to be missing in production builds ...
I agree - I didn't consider the fact that Asserts are disable in
production builds when I made it an assertion.

>> Therefore, the concept of a ResourceOwnerId is introduced. This id identifies
>> the toplevel resourceowner of a transaction, and the owning transaction holds
>> a lock on it just as it does for transaction ids. Porting the first waiting
>> phase to this is trivial - we just collect the RIDs instead of the XIDs of
>> the current lock holders, and wait for them in turn.
>
> I don't particularly like calling these ResourceOwnerId --
> ResourceOwners are an implementation artifact that's purely
> backend-local. If we were to get rid of them in favor of some other
> resource management mechanism, we'd still need RIDs for the waiting
> purposes you mention. So I'm in favor of calling them something else,
> maybe virtual transaction ids (VXID or VID) --- not wedded to that if
> someone has a better idea.
I not particularly proud of the name "ResourceOwnerId" - it was just the
best I could come up with ;-)

I quite like VirtualTransactionId actually - it emphasizes the point
that they never hit the disk. So I'll rename it to VirtualTransactionId
if no better idea comes up.

>> Locks on ResourceOwnerIds are not saved to the 2PC state file on PREPARE, since
>> the RID or a transaction will be reused at time point after it was prepared.
>
> Hmm. I think this is all right since we don't actually need to wait for
> a prepared xact, but it still seems like a wart.
It certainly is a wart, though but removing it will create a bigger one
I fear... The only other I idea I had was to make locks on RIDs session
locks, and to drop them explicitly at toplevel commit and abort time,
and that certainly has a quite high warty-ness score...

> Other than these relatively minor quibbles, the description seems all
> right, and it's so complete that I'm not sure I need to read the code
> ;-)
I *did* warn that I tend to be a bit chatty at times, though ;-)

greetings, Florian Pflug