Re: Load TIME fields - proposed performance improvement

Peter Smith <smithpb2250@gmail.com> writes:
> 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?

That's lost in the mists of time, although one could guess that the
original author preferred to write "tm->somefield" uniformly both
in functions that originate a struct pg_tm and those that receive
a pointer to it.  But nobody has adopted that idea elsewhere in PG,
so it seems like a confusing anachronism to me.

In this particular patch, I got rid of the extra variable in
GetSQLCurrentDate because I was rewriting it pretty completely
anyway, but desisted from doing so in functions that only needed
minor tweaks.  YMMV.

> 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?

Done.

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

I think so too, so I pushed it.

            regards, tom lane