On 2023-03-05 Su 22:18, Amit Langote wrote:
On Tue, Feb 28, 2023 at 8:36 PM Amit Langote <amitlangot...@gmail.com> wrote:
On Mon, Feb 27, 2023 at 4:45 PM Amit Langote <amitlangot...@gmail.com> wrote:
On Tue, Feb 21, 2023 at 2:25 AM Andres Freund <and...@anarazel.de> wrote:
Evaluating N expressions for a json table isn't a good approach, both memory
and CPU efficiency wise.
Are you referring to JsonTableInitOpaque() initializing all these
sub-expressions of JsonTableParent, especially colvalexprs, using N
*independent* ExprStates? That could perhaps be made to work by
making JsonTableParent be an expression recognized by execExpr.c, so
that a single ExprState can store the steps for all its
sub-expressions, much like JsonExpr is. I'll give that a try, though
I wonder if the semantics of making this work in a single
ExecEvalExpr() call will mismatch that of the current way, because
different sub-expressions are currently evaluated under different APIs
of TableFuncRoutine.
Hmm, the idea to turn JSON_TABLE into a single expression turned out
to be a non-starter after all, because, unlike JsonExpr, it can
produce multiple values. So there must be an ExprState for computing
each column of its output rows. As I mentioned in my other reply,
TableFuncScanState has a list of ExprStates anyway for
TableFunc.colexprs. What we could do is move the ExprStates of
TableFunc.colvalexprs into TableFuncScanState instead of making that
part of the JSON_TABLE opaque state, as I've done in the attached
updated patch.
Here's another version in which I've also moved the ExprStates of
PASSING args into TableFuncScanState instead of keeping them in
JSON_TABLE opaque state. That means all the subsidiary ExprStates of
TableFuncScanState are now initialized only once during
ExecInitTableFuncScan(). Previously, JSON_TABLE related ones would be
initialized on every call of JsonTableInitOpaque().
I've also done some cosmetic changes such as renaming the
JsonTableContext to JsonTableParseContext in parse_jsontable.c and to
JsonTableExecContext in jsonpath_exec.c.
Hi, I have just spent some time going through the first five patches
(i.e. those that precede the JSONTABLE patches) and Andres's comments in
<https://postgr.es/m/20230220172456.q3oshnvfk3wyh...@awork3.anarazel.de>
AFAICT there are only two possible matters of concern that remain, both
regarding the grammar.
First is this general complaint:
This stuff adds quite a bit of complexity to the parser. Do we realy need like
a dozen new rules here?
I mentioned that more than a year ago, I think, without anybody taking
the matter up, so I didn't pursue it. I guess I should have.
There are probably some fairly easy opportunities to reduce the number
of non-terminals introduced here (e.g. I think json_aggregate_func could
possibly be expanded in place without introducing
json_object_aggregate_constructor and json_array_aggregate_constructor).
I'm going to make an attempt at that, at least to pick some low hanging
fruit. But in the end I think we are going to be left with a significant
expansion of the grammar rules, more or less forced on us by the way the
SQL Standards Committee rather profligately invents new ways of
contorting the grammar.
Second is this complaint:
+json_behavior_empty_array:
+ EMPTY_P ARRAY { $$ =
makeJsonBehavior(JSON_BEHAVIOR_EMPTY_ARRAY, NULL); }
+ /* non-standard, for Oracle compatibility only */
+ | EMPTY_P { $$ =
makeJsonBehavior(JSON_BEHAVIOR_EMPTY_ARRAY, NULL); }
+ ;
Do we really want to add random oracle compat crud here?
I think this case is pretty harmless, and it literally involves one line
of code, so I'm inclined to leave it.
These both seem like things not worth holding up progress for, and I
think it would be good to get these patches committed as soon as
possible. My intention is to commit them (after some grammar
adjustments) plus their documentation in the next few days. That would
leave the JSONTABLE patches still to go. They are substantial, but a far
more manageable chunk of work for some committer (not me) once we get
this foundational piece in.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
