Re: Extract numeric filed in JSONB more effectively

On 2023-09-04 10:35, Andy Fan wrote:
>   v13 attached.  Changes includes:
> 
> 1.  fix the bug Jian provides.
> 2.  reduce more code duplication without DirectFunctionCall.
> 3.  add the overlooked  jsonb_path_query and jsonb_path_query_first as
> candidates

Apologies for the delay. I like the way this is shaping up.

My first comment will be one that may be too large for this patch
(and too large to rest on my opinion alone); that's why I'm making
it first.

It seems at first a minor point: to me it feels like a wart to have
to pass jsonb_finish_numeric (and *only* jsonb_finish_numeric) a type
oid reflecting the target type of a cast that's going to be applied
*after jsonb_finish_numeric has done its work*, and only for the
purpose of generating a message if the jsonb type *isn't numeric*,
but saying "cannot cast to" (that later target type) instead.

I understand this is done to exactly match the existing behavior,
so what makes this a larger issue is I'm not convinced the existing
behavior is good. Therefore I'm not convinced that bending over
backward to preserve it is good.

What's not good: the places a message from cannotCastJsonbValue
are produced, there has been no attempt yet to cast anything.
The message purely tells you about whether you have the kind
of jsonb node you think you have (and array, bool, null, numeric,
object, string are the only kinds of those). If you're wrong
about what kind of jsonb node it is, you get this message.
If you're right about the kind of node, you don't get this
message, regardless of whether its value can be cast to the
later target type. For example, '32768'::jsonb::int2 produces
ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE "smallint out of range"
but that message comes from the actual int2 cast.

IMV, what the "cannot cast jsonb foo to type %s" message really
means is "jsonb foo where jsonb bar is required" and that's what
it should say, and that message depends on nothing about any
future plans for what will be done to the jsonb bar, so it can
be produced without needing any extra information to be passed.

I'm also not convinced ERRCODE_INVALID_PARAMETER_VALUE is a
good errcode for that message (whatever the wording). I do not
see much precedent elsewhere in the code for using
INVALID_PARAMETER_VALUE to signal this kind of "data value
isn't what you think it is" condition. Mostly it is used
when checking the kinds of parameters passed to a function to
indicate what it should do.

There seem to be several more likely choices for an errcode
there in the 2203x range.

But I understand that issue is not with this patch so much
as with preexisting behavior, and because it's preexisting,
there can be sound arguments against changing it.

But if that preexisting message could be changed, it would
eliminate the need for an unpleasing wart here.

Other notes are more minor:

+        else
+            /* not the desired pattern. */
+            PG_RETURN_POINTER(fexpr);
...
+
+        if (!OidIsValid(new_func_id))
+            PG_RETURN_POINTER(fexpr);
...
+            default:
+                PG_RETURN_POINTER(fexpr);

If I am reading supportnodes.h right, returning NULL is
sufficient to say no transformation is needed.

+        FuncExpr    *fexpr = palloc0(sizeof(FuncExpr));
...
+        memcpy(fexpr, req->fcall, sizeof(FuncExpr));

Is the shallow copy necessary? If so, a comment explaining why
might be apropos. Because the copy is shallow, if there is any
concern about the lifespan of req->fcall, would there not be a
concern about its children?

Is there a reason not to transform the _tz flavors of
jsonb_path_query and jsonb_path-query_first?

-    JsonbValue *v;
-    JsonbValue    vbuf;
+    JsonbValue    *v;
...
+    int i;
      JsonbValue *jbvp = NULL;
-    int            i;

Sometimes it's worth looking over a patch for changes like these,
and reverting such whitespace or position changes, if they aren't
things you want a reviewer to be squinting at. :)

Regards,
-Chap