From: | jian he <jian(dot)universality(at)gmail(dot)com> |
---|---|
To: | Alexandra Wang <alexandra(dot)wang(dot)oss(at)gmail(dot)com> |
Cc: | Nikita Malakhov <hukutoc(at)gmail(dot)com>, Vik Fearing <vik(at)postgresfriends(dot)org>, Mark Dilger <mark(dot)dilger(at)enterprisedb(dot)com>, Matheus Alcantara <matheusssilv97(at)gmail(dot)com>, Peter Eisentraut <peter(at)eisentraut(dot)org>, Andrew Dunstan <andrew(at)dunslane(dot)net>, Nikita Glukhov <glukhov(dot)n(dot)a(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>, "David E(dot) Wheeler" <david(at)justatheory(dot)com> |
Subject: | Re: SQL:2023 JSON simplified accessor support |
Date: | 2025-07-11 05:53:52 |
Message-ID: | CACJufxHAPT6Sh6JW-tU0imFbBkD9kU+2vba-yM44UTLx87ckqQ@mail.gmail.com |
Views: | Whole Thread | Raw Message | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Thu, Jul 10, 2025 at 9:34 PM jian he <jian(dot)universality(at)gmail(dot)com> wrote:
>
> ------------------------------------------
> in jsonb_subscript_make_jsonpath we have
> foreach(lc, *indirection)
> {
> if (IsA(accessor, String))
> ....
> else if (IsA(accessor, A_Indices))
> else
> /*
> * Unsupported node type for creating jsonpath. Instead of
> * throwing an ERROR, break here so that we create a jsonpath from
> * as many indirection elements as we can and let
> * transformIndirection() fallback to alternative logic to handle
> * the remaining indirection elements.
> */
> break;
> }
> the above ELSE branch comments look suspicious to me.
> transformIndirection->transformContainerSubscripts->jsonb_subscript_transform->jsonb_subscript_make_jsonpath
> As you can see, transformIndirection have a long distance from
> jsonb_subscript_make_jsonpath,
> let transformIndirection handle remaining indirection elements seems not good.
>
context v12-0001 to v12-0006.
this ELSE branch comments is wrong, because
+ if (jsonb_check_jsonpath_needed(*indirection))
+ {
+ jsonb_subscript_make_jsonpath(pstate, indirection, sbsref);
+ if (sbsref->refjsonbpath)
+ return;
+ }
in jsonb_check_jsonpath_needed we already use Assert to confirm that
list "indirection"
is either String or A_Indices Node.
in transformContainerSubscripts we have
sbsroutines->transform(sbsref, indirection, pstate,
isSlice, isAssignment);
/*
* Error out, if datatype failed to consume any indirection elements.
*/
if (list_length(*indirection) == indirection_length)
{
Node *ind = linitial(*indirection);
if (noError)
return NULL;
if (IsA(ind, String))
ereport(ERROR,
(errcode(ERRCODE_DATATYPE_MISMATCH),
errmsg("type %s does not support dot notation",
format_type_be(containerType)),
parser_errposition(pstate, exprLocation(containerBase))));
else if (IsA(ind, A_Indices))
ereport(ERROR,
(errcode(ERRCODE_DATATYPE_MISMATCH),
errmsg("type %s does not support array subscripting",
format_type_be(containerType)),
parser_errposition(pstate, exprLocation(containerBase))));
else
elog(ERROR, "invalid indirection operation: %d", nodeTag(ind));
}
sbsroutines->transform currently will call
array_subscript_transform, hstore_subscript_transform, jsonb_subscript_transform
in jsonb_subscript_transform callee we unconditionally do:
*indirection = list_delete_first_n(*indirection, pathlen);
*indirection = list_delete_first_n(*indirection, list_length(upperIndexpr));
in array_subscript_transform, we do
*indirection = list_delete_first_n(*indirection, ndim);
That means, if sbsroutines->transform not error out and indirection is
not NIL (which is unlikely)
then sbsroutines->transform will consume some induction elements.
instead of the above verbose ereport(ERROR, error handling, we can use Assert
Assert(indirection_length > list_length(*indirection));
for the above comments, i did a refactoring based on v12 (0001 to 0006).
Attachment | Content-Type | Size |
---|---|---|
v12-0001-minor-refactor-based-on-v12_0001_to_0006.no-cfbot | application/octet-stream | 3.9 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Peter Smith | 2025-07-11 06:10:00 | Re: [WIP]Vertical Clustered Index (columnar store extension) - take2 |
Previous Message | Dilip Kumar | 2025-07-11 04:19:49 | Re: Adding some error context for lock wait failures |