Обсуждение: Load TIME fields - proposed performance improvement

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.

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

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

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.

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.

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

diff --git a/src/backend/utils/adt/date.c b/src/backend/utils/adt/date.c
index eaaffa7137..057051fa85 100644
--- a/src/backend/utils/adt/date.c
+++ b/src/backend/utils/adt/date.c
@@ -299,20 +299,31 @@ EncodeSpecialDate(DateADT dt, char *str)
 DateADT
 GetSQLCurrentDate(void)
 {
-    TimestampTz ts;
-    struct pg_tm tt,
-               *tm = &tt;
-    fsec_t        fsec;
-    int            tz;
+    struct pg_tm tm;

-    ts = GetCurrentTransactionStartTimestamp();
+    static int    cache_year = 0;
+    static int    cache_mon = 0;
+    static int    cache_mday = 0;
+    static DateADT cache_date;

-    if (timestamp2tm(ts, &tz, tm, &fsec, NULL, NULL) != 0)
-        ereport(ERROR,
-                (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
-                 errmsg("timestamp out of range")));
+    GetCurrentDateTime(&tm);

-    return date2j(tm->tm_year, tm->tm_mon, tm->tm_mday) - POSTGRES_EPOCH_JDATE;
+    /*
+     * date2j involves several integer divisions; moreover, unless our session
+     * lives across local midnight, we don't really have to do it more than
+     * once.  So it seems worth having a separate cache here.
+     */
+    if (tm.tm_year != cache_year ||
+        tm.tm_mon != cache_mon ||
+        tm.tm_mday != cache_mday)
+    {
+        cache_date = date2j(tm.tm_year, tm.tm_mon, tm.tm_mday) - POSTGRES_EPOCH_JDATE;
+        cache_year = tm.tm_year;
+        cache_mon = tm.tm_mon;
+        cache_mday = tm.tm_mday;
+    }
+
+    return cache_date;
 }

 /*
@@ -322,18 +333,12 @@ TimeTzADT *
 GetSQLCurrentTime(int32 typmod)
 {
     TimeTzADT  *result;
-    TimestampTz ts;
     struct pg_tm tt,
                *tm = &tt;
     fsec_t        fsec;
     int            tz;

-    ts = GetCurrentTransactionStartTimestamp();
-
-    if (timestamp2tm(ts, &tz, tm, &fsec, NULL, NULL) != 0)
-        ereport(ERROR,
-                (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
-                 errmsg("timestamp out of range")));
+    GetCurrentTimeUsec(tm, &fsec, &tz);

     result = (TimeTzADT *) palloc(sizeof(TimeTzADT));
     tm2timetz(tm, fsec, tz, result);
@@ -348,18 +353,12 @@ TimeADT
 GetSQLLocalTime(int32 typmod)
 {
     TimeADT        result;
-    TimestampTz ts;
     struct pg_tm tt,
                *tm = &tt;
     fsec_t        fsec;
     int            tz;

-    ts = GetCurrentTransactionStartTimestamp();
-
-    if (timestamp2tm(ts, &tz, tm, &fsec, NULL, NULL) != 0)
-        ereport(ERROR,
-                (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
-                 errmsg("timestamp out of range")));
+    GetCurrentTimeUsec(tm, &fsec, &tz);

     tm2time(tm, fsec, &result);
     AdjustTimeForTypmod(&result, typmod);
diff --git a/src/backend/utils/adt/datetime.c b/src/backend/utils/adt/datetime.c
index dec2fad82a..bc682584f0 100644
--- a/src/backend/utils/adt/datetime.c
+++ b/src/backend/utils/adt/datetime.c
@@ -339,35 +339,78 @@ j2day(int date)
 /*
  * GetCurrentDateTime()
  *
- * Get the transaction start time ("now()") broken down as a struct pg_tm.
+ * Get the transaction start time ("now()") broken down as a struct pg_tm,
+ * according to the session timezone setting.
  */
 void
 GetCurrentDateTime(struct pg_tm *tm)
 {
-    int            tz;
     fsec_t        fsec;

-    timestamp2tm(GetCurrentTransactionStartTimestamp(), &tz, tm, &fsec,
-                 NULL, NULL);
-    /* Note: don't pass NULL tzp to timestamp2tm; affects behavior */
+    /* This is just a convenience wrapper for GetCurrentTimeUsec */
+    GetCurrentTimeUsec(tm, &fsec, NULL);
 }

 /*
  * GetCurrentTimeUsec()
  *
  * Get the transaction start time ("now()") broken down as a struct pg_tm,
- * including fractional seconds and timezone offset.
+ * including fractional seconds and timezone offset.  The time is converted
+ * according to the session timezone setting.
+ *
+ * Callers may pass tzp = NULL if they don't need the offset, but this does
+ * not affect the conversion behavior (unlike timestamp2tm()).
+ *
+ * Internally, we cache the result, since this could be called many times
+ * in a transaction, within which now() doesn't change.
  */
 void
 GetCurrentTimeUsec(struct pg_tm *tm, fsec_t *fsec, int *tzp)
 {
-    int            tz;
+    TimestampTz cur_ts = GetCurrentTransactionStartTimestamp();
+
+    /*
+     * The cache key must include both current time and current timezone.  By
+     * representing the timezone by just a pointer, we're assuming that
+     * distinct timezone settings could never have the same pointer value.
+     * This is true by virtue of the hashtable used inside pg_tzset();
+     * however, it might need another look if we ever allow entries in that
+     * hash to be recycled.
+     */
+    static TimestampTz cache_ts = 0;
+    static pg_tz *cache_timezone = NULL;
+    static struct pg_tm cache_tm;
+    static fsec_t cache_fsec;
+    static int    cache_tz;
+
+    if (cur_ts != cache_ts || session_timezone != cache_timezone)
+    {
+        /*
+         * Make sure cache is marked invalid in case of error after partial
+         * update within timestamp2tm.
+         */
+        cache_timezone = NULL;
+
+        /*
+         * Perform the computation, storing results into cache.  We do not
+         * really expect any error here, since current time surely ought to be
+         * within range, but check just for sanity's sake.
+         */
+        if (timestamp2tm(cur_ts, &cache_tz, &cache_tm, &cache_fsec,
+                         NULL, session_timezone) != 0)
+            ereport(ERROR,
+                    (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+                     errmsg("timestamp out of range")));
+
+        /* OK, so mark the cache valid. */
+        cache_ts = cur_ts;
+        cache_timezone = session_timezone;
+    }

-    timestamp2tm(GetCurrentTransactionStartTimestamp(), &tz, tm, fsec,
-                 NULL, NULL);
-    /* Note: don't pass NULL tzp to timestamp2tm; affects behavior */
+    *tm = cache_tm;
+    *fsec = cache_fsec;
     if (tzp != NULL)
-        *tzp = tz;
+        *tzp = cache_tz;
 }

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

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

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