Re: [PATCH] Lazy xid assingment V2

Gregory Stark wrote:
> "Tom Lane" <tgl@sss.pgh.pa.us> writes:
> 
>> "Florian G. Pflug" <fgp@phlo.org> writes:
>>> Tom Lane wrote:
>>>> 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.
>> Well, I don't see it as arbitrary --- we're trying to hide the fact that
>> XIDs are assigned lazily from user-visible behavior.
> 
> It seems both this and some of the other cases of having to call
> ReadNewTransactionId would be eliminated if we invented a new xid treated
> similarly to current_time() and others which are calculated once per
> transaction and then cached for subsequent accesses. So xid_age() would
> measure relative to a fixed xid, it just wouldn't be *our* xid necessarily.

Hm.. for xid_age that would be OK I think. I'm not so sure about
GetOldestXmin(), though. If we cache the value *before* assigning a
xid, ur cached value might very well turn out to be older than any xmin and
xid in the procarray. GetOldestXmin() would then return a value older
than necessary, which effectively means keeping old data around longer
than necessary. If we only cache the value *after* setting an xmin
it's entirely redundant - we might just use the xid, and be done with it.

I've actually checked who actually uses GetOldestXmin now. The list
is surprisingly short. CreateCheckPoint - To find the pg_subtrans truncation point. Called                    from
bgwriter,and therefore from outside a transaction. IndexBuildHeapScan - Only in the nonconcurrent case. Hardly
           performance-critical. vacuum_set_xid_limits - To decide which tuples are recently dead (Only
        called one per VACUUM). Hardly performance-critical.
 

The reason this list is so short is that we *already* cache a global xmin
value in RecentGlobalXmin. So doing any more optimizations for
GetOldestXmin() seems like overkill. So I'd say lets do the optimization
Tom suggested (Checking GetTopTransactionIdIfAny() before calling
ReadNewTransactionId()), and be done with it.

greetings, Florian Pflug