Re: Load TIME fields - proposed performance improvement

Peter Smith <smithpb2250(at)gmail(dot)com> writes:
> IIUC the code is calling GetCurrentDateTime only to acquire the
> current TX timestamp as a struct pg_tm in order to derive some
> timezone information.
> ...
> I have attached a patch which caches this struct, so now those 225
> million calls are reduced to just 1 call.

Interesting idea, but this implementation is leaving a *lot*
on the table. If we want to cache the result of
timestamp2tm applied to GetCurrentTransactionStartTimestamp(),
there are half a dozen different call sites that could make
use of such a cache, eg, GetSQLCurrentDate and GetSQLCurrentTime.
Applying the caching only in one indirect caller of that seems
pretty narrow-minded.

I'm also strongly suspecting that this implementation is outright
broken, since it's trying to make DecodeTimeOnly's local variable
"tt" into cache storage ... but that variable could be overwritten
with other values, during calls that take the other code path there.
The cache ought to be inside GetCurrentDateTime or something it
calls, and the value needs to be copied to the given output variable.

regards, tom lane