From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: Alexander Korotkov <a.korotkov@postgrespro.ru>
To: Nikita Glukhov <n.gluhov@postgrespro.ru>
Cc: Andres Freund <andres@anarazel.de>
Cc: Oleg Bartunov <obartunov@postgrespro.ru>
Cc: Michael Paquier <michael@paquier.xyz>
Cc: Stas Kelvich <s.kelvich@postgrespro.ru>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>
Cc: Thomas Munro <thomas.munro@enterprisedb.com>
Cc: David Steele <david@pgmasters.net>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Andrew Dunstan <andrew.dunstan@2ndquadrant.com>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: Pavel Stehule <pavel.stehule@gmail.com>
Subject: Re: jsonpath
Date: Tue, 5 Mar 2019 00:27:46 +0100
Message-ID: <67128b8b-2d80-9e51-20a8-9ac8e0e2243e@2ndquadrant.com> (raw)
In-Reply-To: <c25eb8a5-a129-bb7e-c930-761f0e2a6b71@2ndquadrant.com>
References: <CAPpHfdv2bro1224314aRijKJu1bfwEimKr6hExoOaF4sqSDTQA@mail.gmail.com>
<CAPpHfdtxOuEu4t-uGwJF9qzusAmViBf_e3cLYUx1mEoqwWD21g@mail.gmail.com>
<CAPpHfdu4nKqccH0tE84fTJqc7qV-9_w_uic1_zGTYY4FEbQUOw@mail.gmail.com>
<CAPpHfduk=0DOKa40A+0Gi-eo03on_7DZgLZV9vNGdgfUMF_v2w@mail.gmail.com>
<CAPpHfdv+YtPtmircv0kDNH8t9CJ2u8NVGT0JMOxxG2Tdi9BAYg@mail.gmail.com>
<eebc218c-7aaa-c425-6ba0-78b28c751830@2ndquadrant.com>
<9f5f1247-29f4-17cd-edd9-38e9bcd95b14@postgrespro.ru>
<CAPpHfdsxacOREFDAhhZFwL=-x8Z49LCrEK8T9Zn=SJY-EJ+S_Q@mail.gmail.com>
<20190130022826.of7w6xv5lbvzqxfp@alap3.anarazel.de>
<CAPpHfdvov6kWfT3hk8KMSah+kzNuN2AXaZfBqnSU0YkzDVkR2Q@mail.gmail.com>
<957ee2ff-f9a6-0ba1-13e2-9c4d320f63ac@2ndquadrant.com>
<CAPpHfdu+y1tkiidwu0i=XyGK5eOTOu1fPbuYs=XT32g7tzGd4Q@mail.gmail.com>
<a3be6a7a-77d3-0e88-4f9d-4f725d11d7cd@postgrespro.ru>
<CAPpHfdsW924B1eYLqgXE0SkH0ym2Zk32CqPwUu_CH65Gk_onrA@mail.gmail.com>
<c25eb8a5-a129-bb7e-c930-761f0e2a6b71@2ndquadrant.com>
A bunch of additional comments, after looking at the patch a bit today.
All are mostly minor, and sometime perhaps a matter of preference.
1) There's a mismatch between the comment and actual function name for
jsonb_path_match_opr and jsonb_path_exists_opr(). The comments say
"_novars" instead.
2) In a couple of switches the "default" case does a return with a
value, following elog(ERROR). So it's practically unreachable, AFAICS
it's fine without it, and we don't do this elsewhere. And I don't get
any compiler warnings if I remove it either.
Examples:
JsonbTypeName
default:
elog(ERROR, "unrecognized jsonb value type: %d", jbv->type);
return "unknown";
jspOperationName
default:
elog(ERROR, "unrecognized jsonpath item type: %d", type);
return NULL;
compareItems
default:
elog(ERROR, "unrecognized jsonpath operation: %d", op);
return jpbUnknown;
3) jsonpath_send is using makeStringInfo() for a value that is not
returned - IMHO it should use regular stack-allocated variable and use
initStringInfo() instead
4) the version number should be defined/used as a constant, not as a
magic constant somewhere in the code
5) Why does jsonPathToCstring do this?
appendBinaryStringInfo(out, "strict ", 7);
Why not to use regular appendStringInfoString()? What am I missing?
6) comment typo: "Aling StringInfo"
7) alignStringInfoInt() should explain why we need this and why INTALIGN
is the right alignment.
8) I'm a bit puzzled by what flattenJsonPathParseItem does with 'next'
I don't quite understand what it's doing with 'next' value?
/*
* Actual value will be recorded later, after next and children
* processing.
*/
appendBinaryStringInfo(buf,
(char *) &next, /* fake value */
sizeof(next));
Perhaps a comment explaining it (why we need a fake value at all?) would
be a good idea here.
9) I see printJsonPathItem is only calling check_stack_depth while
flattenJsonPathParseItem also calls CHECK_INTERRUPTS. Why the
difference, considering they seem about equally expensive?
10) executeNumericItemMethod is missing a comment (unlike the other
executeXXX functions)
11) Wording of some of the error messages in the execute methods seems a
bit odd. For example executeNumericItemMethod may complain that it
... is applied to not a numeric value
but perhaps a more natural wording would be
... is applied to a non-numeric value
And similarly for the other execute methods. But I'm not a native
speaker, so perhaps the original wording is just fine.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tomas.vondra@2ndquadrant.com, a.korotkov@postgrespro.ru, n.gluhov@postgrespro.ru, andres@anarazel.de, obartunov@postgrespro.ru, michael@paquier.xyz, s.kelvich@postgrespro.ru, thomas.munro@enterprisedb.com, david@pgmasters.net, tgl@sss.pgh.pa.us, andrew.dunstan@2ndquadrant.com, robertmhaas@gmail.com, pavel.stehule@gmail.com
Subject: Re: jsonpath
In-Reply-To: <67128b8b-2d80-9e51-20a8-9ac8e0e2243e@2ndquadrant.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox