Re: Load TIME fields - proposed performance improvement

On Sat, Sep 26, 2020 at 2:17 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> That led me to refactor the patch as attached. (I'd first thought
...

Thanks for refactoring the patch.

Everything looks good to me.

As expected, observations for the v02 patch gave pretty much the same
results as v01 (previous mail).

v02 perf results
2.07% GetSQLCurrentTime
0.50% GetSQLCurrentDate
--.-% GetSQLLocalTime
0.69% GetCurrentDateTime
-.--% GetCurrentTimeUsec

v02 elapsed time
Run1 1m38s
Run2 1m35s
Run3 1m33s

(gives same %19 improvement as v01)

~

I only have a couple of questions, more for curiosity than anything else.

1. Why is there sometimes an extra *tm = &tt; variable introduced?
(e.g. GetSQLCurrentTime, GetSQLLocalTime). Why not just declare struct
pg_tm tm; and pass the &tm same as what GetSQLCurrentDate does?

2. Shouldn't the comment "/* This is just a convenience wrapper for
GetCurrentTimeUsec */" be in the function comment for
GetCurrentDateTime, instead of in the function body?

~

I added a new commitfest entry for this proposal.
https://commitfest.postgresql.org/30/2746/

Is there anything else I should be doing to help get this committed?
IIUC it seems ready as-is.

Kind Regards,
Peter Smith
Fujitsu Australia