Subject: Re: Document DateStyle effect on jsonpath string()

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

On Jul 4, 2024, at 04:28, jian he <[email-masked]> wrote:

> Do you need to reset the datestyle?

Wouldn’t hurt but it’s not necessary, no. It’s set only for the execution of this file, and there are no more calls that rely on it.

> also the above query is time zone sensitive, maybe the time zone is
> set in another place, but that's not explicit?

It’s implicit in how PostgreSQL runs its test suite; other tests later change it.

>        <para>
> -        String value converted from a JSON boolean, number, string, or datetime
> +        String value converted from a JSON boolean, number, string, or
> +        datetime. Note that the string output of datetimes is determined by
> +        the <xref linkend="guc-datestyle"/> parameter.
>        </para>
> imho, your patch has just too many examples.

I’m confused. There are no examples in my patch, or this bit you cite. 

> for explaining the above sentence, the following example should be enough.

Are you referring to the tests? I made them comprehensive so that we reliably demonstrate the behavior of the string() method on all the date/time data types. They are not examples, not in the documentation sense at least.

Best,

David

---

From: jian he <[email-masked]>
Date: Tue, 9 Jul 2024 22:35:09 +0800

> It’s implicit in how PostgreSQL runs its test suite; other tests later change it.

I inserted these two commands into it.
show time zone;
show datestyle;

turns out in the test suite, the default data style is "Postgres,
MDY",  and the default time zone is "PST8PDT".
These two implicit settings weren't mentioned anywhere.

your tests look ok to me.
one tiny complaint would be maybe we need `reset datestyle`.
Because we are in line 600 of src/test/regress/sql/jsonb_jsonpath.sql,
We want to make sure the following test is not influenced by guc datestyle.

> Are you referring to the tests? I made them comprehensive so that we reliably demonstrate the behavior of the string() method on all the date/time data types. They are not examples, not in the docu…
I mean tests, sorry for the confusion. added several more tests should be fine.

overall looks good to me.

---

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

> one tiny complaint would be maybe we need `reset datestyle`.

That’s fair. Done.

D

---

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

>> one tiny complaint would be maybe we need `reset datestyle`.

Here’s a rebase on 5784a49. I also updated the commitfest item[1] to link to a new pull request[2], since I seem to have turned the other one into the tz conversion bug fix.

Best,

David

[1]: https://commitfest.postgresql.org/49/5101/
[2]: https://github.com/theory/postgres/pull/8

---

From: "David E. Wheeler" <[email-masked]>
Date: Tue, 30 Jul 2024 09:47:22 -0400

> [2]: https://github.com/theory/postgres/pull/8

Rebase on 47c9803. I also changed the commitfest item[1] to “ready for committer”, since jian reviewed it, though I couldn’t see a way to add jian as a reviewer in the app. Hope that makes sense.

Best,

David

---

From: Tom Lane <[email-masked]>
Date: Tue, 10 Sep 2024 14:51:48 -0400

> Rebase on 47c9803. I also changed the commitfest item[1] to “ready for committer”, since jian reviewed it, though I couldn’t see a way to add jian as a reviewer in the app. Hope that makes sense.

Pushed with a little additional polishing.

I thought the best way to address jian's complaint about DateStyle not
being clearly locked down was to change horology.sql to verify the
prevailing setting, as it has long done for TimeZone.  That's the
lead test script for related stuff, so it makes the most sense to
do it there.  Having done that, I don't feel a need to duplicate
that elsewhere.

			regards, tom lane

---

From: "David E. Wheeler" <[email-masked]>
Date: Tue, 10 Sep 2024 15:43:09 -0400

> Pushed with a little additional polishing.

Thank you! Do you think it’d be worthwhile to back port to 17?

> do it there.  Having done that, I don't feel a need to duplicate

Yeah, that will help, but I still bet next time I go to figure out what it is I’ll stick that line in some test to make it fail with clear output for what it’s set to 😂.

D

---

From: Peter Eisentraut <[email-masked]>
Date: Tue, 10 Sep 2024 22:10:47 +0200

Isn't this behavior actually a bug that should be fixed rather than 
documented?

These JSON path functions are specified by the SQL standard, so they 
shouldn't depend on PostgreSQL-specific settings.  At least in new 
functionality we should avoid that, no?

---

From: Tom Lane <[email-masked]>
Date: Tue, 10 Sep 2024 16:16:12 -0400

> functionality we should avoid that, no?

Hmm ... but does the standard precisely define the output format?

Since these conversions are built on our own timestamp I/O code,
I rather imagine there is quite a lot of behavior there that's
not to be found in the standard.  That doesn't really trouble
me as long as the spec's behavior is a subset of it (i.e.,
reachable as long as you've got the right parameter settings).

			regards, tom lane

---

From: Tom Lane <[email-masked]>
Date: Tue, 10 Sep 2024 16:17:54 -0400

> Thank you! Do you think it’d be worthwhile to back port to 17?

Not as things stand.  If we adopt Peter's nearby position that
the current behavior is actually buggy, then probably back-patching
a corrected version would be worthwhile as a part of fixing it.

			regards, tom lane

---

From: "David E. Wheeler" <[email-masked]>
Date: Tue, 10 Sep 2024 16:48:27 -0400

On Sep 10, 2024, at 16:10, Peter Eisentraut <[email-masked]> wrote:

> These JSON path functions are specified by the SQL standard, so they shouldn't depend on PostgreSQL-specific settings.  At least in new functionality we should avoid that, no?

Does that also apply to `datetime(template)`, where it uses the `to_timestamp()` templates? From the docs[1]:

> The datetime() and datetime(template) methods use the same parsing rules as the to_timestamp SQL function does (see Section 9.8[2]), with three exceptions. First, these methods don't allow unmatched template patterns. Second, only the following separators are allowed in the template string: minus sign, period, solidus (slash), comma, apostrophe, semicolon, colon and space. Third, separators in the template string must exactly match the input string.

Does the standard specify a formatting language?

Best,

David

[1]: https://www.postgresql.org/docs/devel/functions-json.html
[2]: https://www.postgresql.org/docs/devel/functions-formatting.html

---

From: "David E. Wheeler" <[email-masked]>
Date: Tue, 10 Sep 2024 16:50:06 -0400

> a corrected version would be worthwhile as a part of fixing it.

Oh, I see now that my reply to him points out the same issue as yours.

So annoying that the standard is not publicly available for any one of us to go look.

D

---

From: Peter Eisentraut <[email-masked]>
Date: Wed, 11 Sep 2024 12:20:41 +0200

> reachable as long as you've got the right parameter settings).

Actually, the standard prohibits this call:

"""
XV) If JM specifies string, then:

1) Forallj,1(one)≤j≤n,
Case:

a) If Ij is not a character string, number, or Boolean value,
then let ST be data exception — non-string SQL/JSON item (2202X).

b) Otherwise, let X be an SQL variable whose value is Ij. Let ML be an 
implementation-defined (IL006) maximum
length of variable-length character strings. Let Vj be the result of

CAST (X AS CHARACTER VARYING(ML)

If this conversion results in an exception condition, then
let ST be that exception condition.
"""

So I guess we have extended this and the current behavior is consistent 
with item b).

What I'm concerned about is that this makes the behavior of JSON_QUERY 
non-immutable.  Maybe there are other reasons for it to be 
non-immutable, in which case this isn't important.  But it might be 
worth avoiding that?