pgsql: Implement jsonpath .datetime() method

On Wed, Sep 25, 2019 at 10:52 PM Alexander Korotkov
<akorotkov@postgresql.org> wrote:

Implement jsonpath .datetime() method

I've noticed buildfarm is unhappy with my commits.

1. https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=lapwing&amp;dt=2019-09-25%2020%3A40%3A11
2. https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=dromedary&amp;dt=2019-09-25%2020%3A38%3A21

I'm investigating the issue.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Thu, Sep 26, 2019 at 12:05 AM Alexander Korotkov <
a.korotkov@postgrespro.ru> wrote:

On Wed, Sep 25, 2019 at 10:52 PM Alexander Korotkov
<akorotkov@postgresql.org> wrote:

Implement jsonpath .datetime() method

I've noticed buildfarm is unhappy with my commits.

1.

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=lapwing&amp;dt=2019-09-25%2020%3A40%3A11

2.

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=dromedary&amp;dt=2019-09-25%2020%3A38%3A21

Appears to be another issue happening when
USE_FLOAT8_BYVAL is undefined. Managed to reproduce it locally.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Alexander Korotkov <a.korotkov@postgrespro.ru> writes:

I've noticed buildfarm is unhappy with my commits.

The proximate problem seems to be that compareItems() is insufficiently
careful to ensure that both values are non-null before passing them
off to datatype-specific code. The code accidentally fails to crash
on 64-bit machines, but it's still giving garbage answers, I think.

regards, tom lane

On Thu, Sep 26, 2019 at 2:12 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Alexander Korotkov <a.korotkov@postgrespro.ru> writes:

I've noticed buildfarm is unhappy with my commits.

The proximate problem seems to be that compareItems() is insufficiently
careful to ensure that both values are non-null before passing them
off to datatype-specific code. The code accidentally fails to crash
on 64-bit machines, but it's still giving garbage answers, I think.

I've found compareItems() code to not apply appropriate cast to/from
Datum. Fixed in 7881bb14f4. This makes test pass on my local 32-bit
machine. I'll keep look on buildfarm.

It seems that caller (executePredicate()) don't NULL values to
compareItems(). It does do only for predicates, which doesn't have
right argument. But binary predicate with lacking argument should be
detected during syntactic analysis.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Alexander Korotkov <a.korotkov@postgrespro.ru> writes:

On Thu, Sep 26, 2019 at 2:12 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

The proximate problem seems to be that compareItems() is insufficiently
careful to ensure that both values are non-null before passing them
off to datatype-specific code. The code accidentally fails to crash
on 64-bit machines, but it's still giving garbage answers, I think.

I've found compareItems() code to not apply appropriate cast to/from
Datum. Fixed in 7881bb14f4. This makes test pass on my local 32-bit
machine. I'll keep look on buildfarm.

Hm. dromedary seems not to crash either with that fix, but I'm not
sure why not, because when I was running the previous tree by hand,
the stack trace showed pretty clearly that we were getting to
timestamp_cmp with one null and one non-null argument. So I don't
believe your argument that that's impossible, and even if it is,
I do not think it's sane for compareItems to depend on that ---
especially when one of its code paths *does* check for nulls.

I do not have a very good opinion about the quality of this code
upon my first glance at it. Just looking at compareDatetime:

* The code is schizophrenic about whether it's allowed to pass a
null have_error pointer or not. It is not very sensible to have
some code doing
if (have_error && *have_error)
return 0;
when other code branches will dump core for null have_error.
Given that if this test actually was doing anything, what it
would be doing is failing to detect error conditions, I think
the only sensible design is to insist that have_error mustn't be
null, in which case these checks for null pointer should be removed,
because (a) they waste code and cycles and (b) they mislead the
reader as to what the API of compareDatetime actually is.

* At least some of the code paths will malfunction if the caller
didn't initialize *have_error to false. If that is an intended API
requirement, it is not acceptable for the function header comment
not to say so. (For bonus points, it'd be nice if the header
comment agreed with the code as to the name of the variable.)
If this isn't an intended requirement, you need to fix the code,
probably by initializing "*have_error = false;" up at the top.

* It's a bit schizophrenic also that some of the switches
lack default:s (and rely on the if (!cmpfunc) below), while
the outer switch does have its own, completely redundant
default:. I'd get rid of that default: and instead add
a comment explaining that the !cmpfunc test substitutes for
default branches.

* This is silly:

if (*have_error)
return 0;

*have_error = false;

* Also, given that you have that "if (*have_error)" where you do,
the have_error tests inside the switch are useless redundancy.
You might as well just remove them completely and let the final
test handle falling out if a conversion failed. Alternatively
you could drop the final test, because as the code stands right
now, it's visibly impossible to get there with *have_error true.

* More generally, it's completely unclear why some error conditions
are thrown as errors and others just result in returning *have_error.
In particular, it seems weird that some unsupported datatype combinations
cause hard errors while others do not. Maybe that's fine, but if so,
the function header comment is falling down on the job by not explaining
the reasoning.

* OIDs are unsigned, so if you must print them, use %u not %d.

* The errhints don't follow project message style.

* The blank lines before "break"s aren't really per project
style either, IMO. They certainly aren't doing anything to
improve readability, and they help limit how much code you
can see at once.

regards, tom lane

On Thu, Sep 26, 2019 at 2:57 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Alexander Korotkov <a.korotkov@postgrespro.ru> writes:

On Thu, Sep 26, 2019 at 2:12 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

The proximate problem seems to be that compareItems() is insufficiently
careful to ensure that both values are non-null before passing them
off to datatype-specific code. The code accidentally fails to crash
on 64-bit machines, but it's still giving garbage answers, I think.

I've found compareItems() code to not apply appropriate cast to/from
Datum. Fixed in 7881bb14f4. This makes test pass on my local 32-bit
machine. I'll keep look on buildfarm.

Hm. dromedary seems not to crash either with that fix, but I'm not
sure why not, because when I was running the previous tree by hand,
the stack trace showed pretty clearly that we were getting to
timestamp_cmp with one null and one non-null argument. So I don't
believe your argument that that's impossible, and even if it is,
I do not think it's sane for compareItems to depend on that ---
especially when one of its code paths *does* check for nulls.

I do not have a very good opinion about the quality of this code
upon my first glance at it. Just looking at compareDatetime:

* The code is schizophrenic about whether it's allowed to pass a
null have_error pointer or not. It is not very sensible to have
some code doing
if (have_error && *have_error)
return 0;
when other code branches will dump core for null have_error.
Given that if this test actually was doing anything, what it
would be doing is failing to detect error conditions, I think
the only sensible design is to insist that have_error mustn't be
null, in which case these checks for null pointer should be removed,
because (a) they waste code and cycles and (b) they mislead the
reader as to what the API of compareDatetime actually is.

* At least some of the code paths will malfunction if the caller
didn't initialize *have_error to false. If that is an intended API
requirement, it is not acceptable for the function header comment
not to say so. (For bonus points, it'd be nice if the header
comment agreed with the code as to the name of the variable.)
If this isn't an intended requirement, you need to fix the code,
probably by initializing "*have_error = false;" up at the top.

* It's a bit schizophrenic also that some of the switches
lack default:s (and rely on the if (!cmpfunc) below), while
the outer switch does have its own, completely redundant
default:. I'd get rid of that default: and instead add
a comment explaining that the !cmpfunc test substitutes for
default branches.

* This is silly:

if (*have_error)
return 0;

*have_error = false;

* Also, given that you have that "if (*have_error)" where you do,
the have_error tests inside the switch are useless redundancy.
You might as well just remove them completely and let the final
test handle falling out if a conversion failed. Alternatively
you could drop the final test, because as the code stands right
now, it's visibly impossible to get there with *have_error true.

* More generally, it's completely unclear why some error conditions
are thrown as errors and others just result in returning *have_error.
In particular, it seems weird that some unsupported datatype combinations
cause hard errors while others do not. Maybe that's fine, but if so,
the function header comment is falling down on the job by not explaining
the reasoning.

* OIDs are unsigned, so if you must print them, use %u not %d.

* The errhints don't follow project message style.

* The blank lines before "break"s aren't really per project
style either, IMO. They certainly aren't doing anything to
improve readability, and they help limit how much code you
can see at once.

Thank you for feedback. Nikita and me will provide patches to fix
pointed issues during next couple of days.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company