Subject: Re: jsonpath: Inconsistency of timestamp_tz() Output

From: "David E. Wheeler" <[email-masked]>
Date: Tue, 9 Jul 2024 10:07:48 -0400

> On Jul 8, 2024, at 21:44, Junwang Zhao <[email-masked]> wrote:
> 
> # select jsonb_path_query_tz('"2024-08-15 12:34:56-05"', '$.timestamp_tz()');
> 
> Do you also expect this to show the time in America/New_York?
> 
> This is what I get:
> 
> [local] postgres@postgres:5432-28176=# select
> jsonb_path_query_tz('"2024-08-15 12:34:56-05"', '$.timestamp_tz()');
> ┌─────────────────────────────┐
> │     jsonb_path_query_tz     │
> ├─────────────────────────────┤
> │ "2024-08-15T12:34:56-05:00" │
> └─────────────────────────────┘
> (1 row)
> 
> The logic in executeDateTimeMethod seems to convert the input to a UTC
> timestamp base on the session TZ,
> the output seems not cast based on the TZ.

Right, which now that I think about it seems odd, because timestamptz does not actually store an offset. As you say, it converts the time to UTC and stores only that, then displays the offset relative to the current time zone setting.

So in plain SQL it always displays relative to the current TZ offset:

david=# set time zone 'America/New_York';
SET
david=# select '2023-08-15 12:34:56-05'::timestamptz;
      timestamptz       
------------------------
 2023-08-15 13:34:56-04
(1 row)

david=# select '2023-08-15 12:34:56'::timestamptz;
      timestamptz       
------------------------
 2023-08-15 12:34:56-04
(1 row)

In jsopath expressions, however, it does not, as your example demonstrates:

david=# select jsonb_path_query_tz('"2024-08-15 12:34:56-05"', '$.timestamp_tz()');
     jsonb_path_query_tz     
-----------------------------
 "2024-08-15T12:34:56-05:00"

How is it retaining the offset? Should it?

The display is properly adjusted when using string():

david=# select jsonb_path_query_tz('"2024-08-15 12:34:56-05"', '$.timestamp_tz().string()');
   jsonb_path_query_tz    
--------------------------
 "2024-08-15 13:34:56-04"
(1 row)

So perhaps I had things reversed before. Maybe it’s actually doing the right then when it converts a timestamp to a timestamptz, but not when it the input contains an offset, as in your example.

D

---

From: "David E. Wheeler" <[email-masked]>
Date: Tue, 9 Jul 2024 10:22:03 -0400

> So perhaps I had things reversed before. Maybe it’s actually doing the right then when it converts a timestamp to a timestamptz, but not when it the input contains an offset, as in your example.

To clarify, there’s an inconsistency in the output of timestamp_tz() depending on whether the input has an offset or not. With offset:

And without:

david=# select jsonb_path_query_tz('"2024-08-15 12:34:56"', '$.timestamp_tz()');
     jsonb_path_query_tz     
-----------------------------
 "2024-08-15T16:34:56+00:00"

I suspect the latter is correct, given that the timestamptz type appears to be an int64, presumably always in UTC. I don’t understand where the first example stores the offset.

Best,

David

---

From: Junwang Zhao <[email-masked]>
Date: Tue, 9 Jul 2024 23:08:52 +0800

> david=# select jsonb_path_query_tz('"2024-08-15 12:34:56-05"', '$.timestamp_tz()');

In JsonbValue.val.datatime, there is a tz field, I think that's where
the offset stored, it is 18000 in the first example

struct
{
  Datum value;
  Oid typid;
  int32 typmod;
  int tz; /* Numeric time zone, in seconds, for
            * TimestampTz data type */
} datetime;

-- 
Regards
Junwang Zhao

---

From: "David E. Wheeler" <[email-masked]>
Date: Tue, 9 Jul 2024 11:38:02 -0400

>          * TimestampTz data type */

Oooh, okay, so it’s a jsonb variant of the type. Interesting. Ah, and it’s assigned here[1]:

  jb->val.datetime.tz = tz;

It seems like JSONB timestamptz values want to display the recorded time zone, so I suspect we need to set it when the converting from a non-tz to a local tz setting, something like this:

``` patch
diff --git a/src/backend/utils/adt/jsonpath_exec.c b/src/backend/utils/adt/jsonpath_exec.c
index d79c929822..f63b3b9330 100644
--- a/src/backend/utils/adt/jsonpath_exec.c
+++ b/src/backend/utils/adt/jsonpath_exec.c
@@ -2707,12 +2707,16 @@ executeDateTimeMethod(JsonPathExecContext *cxt, JsonPathItem *jsp,
			break;
		case jpiTimestampTz:
			{
+				struct pg_tm *tm;
				/* Convert result type to timestamp with time zone */
				switch (typid)
				{
					case DATEOID:
						checkTimezoneIsUsedForCast(cxt->useTz,
												   "date", "timestamptz");
+						if (timestamp2tm(DatumGetTimestamp(value), NULL, tm, NULL, NULL, NULL) == 0) {
+							tz = DetermineTimeZoneOffset(tm, session_timezone);
+						}
						value = DirectFunctionCall1(date_timestamptz,
													value);
						break;
@@ -2726,6 +2730,9 @@ executeDateTimeMethod(JsonPathExecContext *cxt, JsonPathItem *jsp,
					case TIMESTAMPOID:
						checkTimezoneIsUsedForCast(cxt->useTz,
												   "timestamp", "timestamptz");
+						if (timestamp2tm(DatumGetTimestamp(value), NULL, tm, NULL, NULL, NULL) == 0) {
+							tz = DetermineTimeZoneOffset(tm, session_timezone);
+						}
						value = DirectFunctionCall1(timestamp_timestamptz,
													value);
						break;
```

Only, you know, doesn’t crash the server.

Best,

David

[1]: https://github.com/postgres/postgres/blob/629520be5f9da9d0192c7f6c8796bfddb4746760/src/backend/utils/adt/jsonpath_exec.c#L2784

---

From: Junwang Zhao <[email-masked]>
Date: Wed, 10 Jul 2024 13:48:17 +0800

> Only, you know, doesn’t crash the server.

I apply your patch with some minor change(to make the server not crash):

diff --git a/src/backend/utils/adt/jsonpath_exec.c
b/src/backend/utils/adt/jsonpath_exec.c
index d79c9298227..87a695ef633 100644
--- a/src/backend/utils/adt/jsonpath_exec.c
+++ b/src/backend/utils/adt/jsonpath_exec.c
@@ -2708,6 +2708,8 @@ executeDateTimeMethod(JsonPathExecContext *cxt,
JsonPathItem *jsp,
                case jpiTimestampTz:
                        {
                                /* Convert result type to timestamp
with time zone */
+                               struct pg_tm tm;
+                               fsec_t fsec;
                                switch (typid)
                                {
                                        case DATEOID:
@@ -2726,6 +2728,9 @@ executeDateTimeMethod(JsonPathExecContext *cxt,
JsonPathItem *jsp,
                                        case TIMESTAMPOID:

                            "timestamp", "timestamptz");
+                                               if
(timestamp2tm(DatumGetTimestamp(value), NULL, &tm, &fsec, NULL, NULL)
== 0) {
+                                                       tz =
DetermineTimeZoneOffset(&tm, session_timezone);
+                                               }
                                                value =
DirectFunctionCall1(timestamp_timestamptz,

                                 value);
                                                break;

It now gives the local tz:

[local] postgres@postgres:5432-54960=# set time zone 'America/New_York';
SET
Time: 2.894 ms
[local] postgres@postgres:5432-54960=# select
jsonb_path_query_tz('"2024-08-15 12:34:56"', '$.timestamp_tz()');
┌─────────────────────────────┐
│     jsonb_path_query_tz     │
├─────────────────────────────┤
│ "2024-08-15T12:34:56-04:00" │
└─────────────────────────────┘
(1 row)

Time: 293813.022 ms (04:53.813)

I'm not sure whether the SQL/JSON standard mentioned this, I searched a
little bit, but found no clue :(

-- 
Regards
Junwang Zhao

---

From: "David E. Wheeler" <[email-masked]>
Date: Wed, 10 Jul 2024 10:33:05 -0400

> I apply your patch with some minor change(to make the server not crash):

Oh, thank you! Kicking myself for not catching the obvious.

> jsonb_path_query_tz('"2024-08-15 12:34:56"', '$.timestamp_tz()');

Yes, and I think that’s what we want, since it preserves and displays the offset for strings that contain them:

    david=# set time zone 'America/New_York';
    SET
    david=# select jsonb_path_query_tz('"2024-08-15 12:34:56+10"', '$.timestamp_tz()');
        jsonb_path_query_tz
    -----------------------------
    "2024-08-15T12:34:56+10:00"

> little bit, but found no clue :(

Yeah I don’t know either, but now at least it’s consistent. I’ve attached a patch to fix it.

Ideally, I think, we wouldn’t convert the value and determine the offset twice, but teach date_timestamptz and timestamp_timestamptz (or date2timestamptz and timestamp2timestamptz?) how to return the offset, or create alternate functions that do so. Not sure what calling style should be adopted here, but this at least addresses the issue. Happy to resubmit something more efficient upon function design feedback.

Best,

David

---

From: "David E. Wheeler" <[email-masked]>
Date: Wed, 10 Jul 2024 10:54:27 -0400

> Yeah I don’t know either, but now at least it’s consistent. I’ve attached a patch to fix it.

Actually I think there’s a subtlety still missing here:

@@ -2914,7 +2914,7 @@ HINT:  Use *_tz() function for time zone support.
 select jsonb_path_query_tz('"2023-08-15"', '$.timestamp_tz()'); -- should work
      jsonb_path_query_tz       -----------------------------
- "2023-08-15T07:00:00+00:00"
+ "2023-08-14T23:00:00-08:00"
 (1 row)

This test runs while the time zone is set to “PST8PDT”, but it’s got the PST offset when it should be PDT:

david=# set time zone 'PST8PDT';
SET
david=# select '2023-08-15'::timestamptz;
      timestamptz       
------------------------
 2023-08-15 00:00:00-07

So it should be -7, not -8. Not sure where to tell it to pay proper attention to daylight savings time.

Best,

David

---

From: "David E. Wheeler" <[email-masked]>
Date: Wed, 10 Jul 2024 11:19:32 -0400

> So it should be -7, not -8. Not sure where to tell it to pay proper attention to daylight savings time.

Oh, and the time and date were wrong, too, because I blindly used the same conversion for dates as for timestamps. Fixed in v2.

PR: https://github.com/theory/postgres/pull/7
CF: https://commitfest.postgresql.org/49/5119/

Best,

David

---

From: "David E. Wheeler" <[email-masked]>
Date: Fri, 19 Jul 2024 10:05:25 -0400

> CF: https://commitfest.postgresql.org/49/5119/

Rebase on 5784a49. No other changes. I would consider this a bug in features added for 17.

Best,

David