Re: Load TIME fields - proposed performance improvement

Peter Smith <smithpb2250@gmail.com> writes:
> The patch has been re-implemented based on previous advice.
> Please see attached.

Hm, so:

1. The caching behavior really needs to account for the possibility of
the timezone setting being changed intra-transaction.  That's not very
likely, perhaps, but we can't deliver a wrong answer.  It seems best
to make the dependency on session_timezone explicit instead of
allowing it to be hidden inside timestamp2tm.

2. I'd had in mind just one cache, not several.  Admittedly it
doesn't gain that much for different code paths to share the result,
but it seems silly not to, especially given the relative complexity
of getting the caching right.

That led me to refactor the patch as attached.  (I'd first thought
that we could just leave the j2date calculation in GetSQLCurrentDate
out of the cache, but performance measurements showed that it is
worthwhile to cache that.  An advantage of the way it's done here
is we probably won't have to redo j2date even across transactions.)

> (represents 19% improvement for this worst case table/data)

I'm getting a hair under 30% improvement on this admittedly
artificial test case, for either your patch or mine.

> Note: I patched the function GetCurrentTimeUsec consistently with the
> others, but actually I was not able to discover any SQL syntax which
> could cause that function to be invoked multiple times. Perhaps the
> patch for that function should be removed?

The existing callers for that are concerned with interpreting "now"
in datetime input strings.  It's certainly possible to have to do that
more than once per transaction --- an example would be COPY IN where
a lot of rows contain "now" as the value of a datetime column.

			regards, tom lane