From: | Aleksander Alekseev <aleksander(at)timescale(dot)com> |
---|---|
To: | pgsql-hackers(at)lists(dot)postgresql(dot)org |
Cc: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Daniel Gustafsson <daniel(at)yesql(dot)se>, Peter Smith <smithpb2250(at)gmail(dot)com>, David Steele <david(at)pgmasters(dot)net> |
Subject: | Re: Support TZ format code in to_timestamp() |
Date: | 2024-01-23 13:26:48 |
Message-ID: | CAJ7c6TOPsuzM0AwO1X_Fr077ZFYGi7Pg=TKEtB230f8n=yAr4w@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Hi,
> Since I had this on my (ever-growing) TODO I re-prioritized today and took a
> look at it since I think it's something we should support.
>
> Nothing really sticks out and I was unable to poke any holes so I don't have
> too much more to offer than a LGTM.
>
> + while (len > 0)
> + {
> + const datetkn *tp = datebsearch(lowtoken, zoneabbrevtbl->abbrevs,
> + zoneabbrevtbl->numabbrevs);
>
> My immediate reaction was that we should stop at prefix lengths of two since I
> could only think of abbreviations of two or more. Googling and reading found
> that there are indeed one-letter timezones (Alpha, Bravo etc..). Not sure if
> it's worth mentioning that in the comment to help other readers who aren't neck
> deep in timezones?
>
> + /* FALL THRU */
>
> Tiny nitpick, it looks a bit curious that we spell it FALL THRU here and "fall
> through" a few cases up in the same switch. While we are quite inconsistent
> across the tree, consistency within a file is preferrable (regardless of
> which).
I reviewed the patch and tested it on MacOS and generally concur with
stated above. The only nitpick I have is the apparent lack of negative
tests for to_timestamp(), e.g. when the string doesn't match the
specified format.
--
Best regards,
Aleksander Alekseev
From | Date | Subject | |
---|---|---|---|
Next Message | Amit Langote | 2024-01-23 13:46:06 | Re: remaining sql/json patches |
Previous Message | Aleksander Alekseev | 2024-01-23 13:10:40 | Re: FEATURE REQUEST: Role vCPU limit/priority |