Re: SQL:2023 JSON simplified accessor support

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-06-25 05:56:51
Message-ID: CACJufxFAaJ3=X7wnGmS0857ia8+-iwDzxhua9X8Qnh_CVB1V1A@mail.gmail.com
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

hi.

in src/backend/catalog/sql_features.txt
should we mark any T860, T861, T862, T863, T864
items as YES?

typedef struct SubscriptingRef
{
/* expressions that evaluate to upper container indexes */
List *refupperindexpr;
}
SubscriptingRef.refupperindexpr meaning changed,
So the above comments also need to be changed?

v11-0004-Extract-coerce_jsonpath_subscript.patch
+ /*
+ * We known from can_coerce_type that coercion will succeed, so
+ * coerce_type could be used. Note the implicit coercion context, which is
+ * required to handle subscripts of different types, similar to overloaded
+ * functions.
+ */
+ subExpr = coerce_type(pstate,
+ subExpr, subExprType,
+ targetType, -1,
+ COERCION_IMPLICIT,
+ COERCE_IMPLICIT_CAST,
+ -1);
+ if (subExpr == NULL)
+ ereport(ERROR,
+ (errcode(ERRCODE_DATATYPE_MISMATCH),
+ errmsg("jsonb subscript must have text type"),

the targetType can be "integer", then the error message
errmsg("jsonb subscript must have text type") would be wrong?
Also this error handling is not necessary.
since we can_coerce_type already tell us that coercion will succeed.
Also, do we really put v11-0004 as a separate patch?

in gram.y we have:
if (!IsA($5, A_Const) ||
castNode(A_Const, $5)->val.node.type != T_String)
ereport(ERROR,
errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("only string constants are
supported in JSON_TABLE path specification"),
so simply, in make_jsonpath_item_expr we can

expr = transformExpr(pstate, expr, pstate->p_expr_kind);
if (!IsA(expr, Const) || ((Const *) expr)->consttype != INT4OID)
ereport(ERROR,
errcode(ERRCODE_DATATYPE_MISMATCH),
errmsg("only integer constants are supported in jsonb
simplified accessor subscripting"),
parser_errposition(pstate, exprLocation(expr)));

because I think the current error message "got type: unknown" is not good.
select ('123'::jsonb).a['1'];
ERROR: jsonb simplified accessor supports subscripting in type: INT4,
got type: unknown
then we don't need two "ereport(ERROR" within make_jsonpath_item_expr
we can also Assert (cnst->constisnull) is false.
see gram.y:16976

I saw you introduce the word "AST", for example
"Converts jsonpath AST into jsonpath value in binary."
I am not sure that is fine.

in jsonb_subscript_make_jsonpath we have:
+ foreach(lc, *indirection)
+ {
+
+ if (IsA(accessor, String))
+ ....
+ else if (IsA(accessor, A_Star))
+ ....
+ else if (IsA(accessor, A_Indices))
+ ....
+ else
+ break;

Is the last else branch unreliable? since indirection only support for
String, A_Star, A_Indices, we already have Assert in jsonb_check_jsonpath_needed
to ensure that.
+ *indirection = list_delete_first_n(*indirection, pathlen);
also this is not necessary,
because pathlen will be the same length as list *indirection in current design.

Please check the attached minor refactoring based on v11-0001 to v11-0006

Attachment Content-Type Size
v11-0001-misc-refactoring-based-on-v11_01_to_06.no-cfbot application/octet-stream 3.7 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Michael Banck 2025-06-25 06:04:59 Re: Safeguards against incorrect fd flags for fsync()
Previous Message Dilip Kumar 2025-06-25 05:33:39 Re: Logrep launcher race conditions leading to slow tests