Horton

Thread

Commits

  1. Cache the result of converting now() to a struct pg_tm.

  1. Load TIME fields - proposed performance improvement

    Peter Smith <smithpb2250@gmail.com> — 2020-09-22T01:42:55Z

    Hi Hackers.

I have a test table with multiple (10) columns defined as TIME WITHOUT
TIME ZONE.

When loading this table with a lot of data (e.g. "COPY tbl FROM
/my/path/2GB.csv WITH (FORMAT CSV)") I observed it was spending an
excessive amount of time within the function GetCurrentDateTime.

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.

My test table has 10 x TIME columns.
My test data has 22.5 million rows (~ 2GB)
So that's 225 million times the GetCurrentDateTime function is called
to populate the struct with the same values.

I have attached a patch which caches this struct, so now those 225
million calls are reduced to just 1 call.

~

Test Results:

Copy 22.5 million rows data (~ 2GB)

BEFORE
Run 1 = 4m 36s
Run 2 = 4m 30s
Run 3 = 4m 32s
perf showed 20.95% time in GetCurrentDateTime

AFTER (cached struct)
Run 1 = 3m 44s
Run 2 = 3m 44s
Run 3 = 3m 45s
perf shows no time in GetCurrentDateTime
~17% performance improvement in my environment. YMMV.

~

Thoughts?

Kind Regards
Peter Smith.
Fujitsu Australia.

  2. Re: Load TIME fields - proposed performance improvement

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-09-22T02:12:45Z

    Peter Smith <smithpb2250@gmail.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

  3. Re: Load TIME fields - proposed performance improvement

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-09-22T02:44:27Z

    I wrote:
> 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.

As an example, I did some quick-and-dirty "perf" measurement of
this case:

create table t1 (id int, d date default current_date);
insert into t1 select generate_series(1,100000000);

and found that about 10% of the runtime is spent inside timestamp2tm().
Essentially all of that cost could be removed by a suitable caching
patch.  Admittedly, this is a pretty well cherry-picked example, and
more realistic test scenarios might see just a percent or two win.
Still, for the size of the patch I'm envisioning, it'd be well
worth the trouble.

			regards, tom lane

  4. Re: Load TIME fields - proposed performance improvement

    Peter Smith <smithpb2250@gmail.com> — 2020-09-22T03:06:15Z

    Hi Tom.

Thanks for your feedback.

On Tue, Sep 22, 2020 at 12:44 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Still, for the size of the patch I'm envisioning, it'd be well
> worth the trouble.

The OP patch I gave was just a POC to test the effect and to see if
the idea was judged as worthwhile...

I will rewrite/fix it based on your suggestions.

Kind Regards,
Peter Smith.
Fujitsu Australia.

  5. Re: Load TIME fields - proposed performance improvement

    Peter Smith <smithpb2250@gmail.com> — 2020-09-25T06:34:03Z

    The patch has been re-implemented based on previous advice.

Please see attached.

~

Test:

A test table was created and 20 million rows inserted as follows:

test=# create table t1 (id int, a timestamp, b time without time zone
default '01:02:03', c date default CURRENT_DATE, d time with time zone
default CURRENT_TIME, e time with time zone default LOCALTIME);
CREATE TABLE

$ time psql -d test -c "insert into t1(id, a)
values(generate_series(1,20000000), timestamp 'now');"

~

Observations:

BEFORE PATCH

perf results
6.18% GetSQLCurrentTime
5.73% GetSQLCurrentDate
5.20% GetSQLLocalTime
4.67% GetCurrentDateTime
-.--% GetCurrentTimeUsec

elapsed time
Run1 1m57s
Run2 1m58s
Run3 2m00s

AFTER PATCH

perf results
1.77% GetSQLCurrentTime
0.12% GetSQLCurrentDate
0.50% GetSQLLocalTime
0.36% GetCurrentDateTime
-.--% GetCurrentTimeUsec

elapsed time
Run1 1m36s
Run2 1m36s
Run3 1m36s

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

~

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?

---

Kind Regards,
Peter Smith
Fujitsu Australia

On Tue, Sep 22, 2020 at 1:06 PM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Hi Tom.
>
> Thanks for your feedback.
>
> On Tue, Sep 22, 2020 at 12:44 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> > Still, for the size of the patch I'm envisioning, it'd be well
> > worth the trouble.
>
> The OP patch I gave was just a POC to test the effect and to see if
> the idea was judged as worthwhile...
>
> I will rewrite/fix it based on your suggestions.
>
> Kind Regards,
> Peter Smith.
> Fujitsu Australia.

  6. Re: Load TIME fields - proposed performance improvement

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-09-25T16:17:05Z

    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

  7. Re: Load TIME fields - proposed performance improvement

    Peter Smith <smithpb2250@gmail.com> — 2020-09-28T02:51:40Z

    On Sat, Sep 26, 2020 at 2:17 AM Tom Lane <tgl@sss.pgh.pa.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

  8. Re: Load TIME fields - proposed performance improvement

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-09-28T16:10:02Z

    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

  9. Re: Load TIME fields - proposed performance improvement

    Peter Smith <smithpb2250@gmail.com> — 2020-09-29T01:26:58Z

    On Tue, Sep 29, 2020 at 2:10 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I think so too, so I pushed it.

Thanks!

Kind Regards,
Peter Smith
Fujitsu Australia