Re: Extract numeric filed in JSONB more effectively

On Thu, Sep 14, 2023 at 5:18 AM Chapman Flack <chap(at)anastigmatix(dot)net> wrote:

> 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.

This is a great signal:)

>
>
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.
>

I hesitated to do so, but I'm thinking if any postgresql user uses
some code like if (errMsg.equals('old-error-message')), and if we
change the error message, the application will be broken. I agree
with the place for the error message, IIUC, you intend to choose
not compatible with the old error message?

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.
>

I double confirmed you are right here.
Changed it to PG_RETURN_POINT(null); here in the next version.

>
> + 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?
>

All the interesting parts in the input FuncExpr are heap based,
but the FuncExpr itself is stack based (I'm not sure why the fact
works like this), Also based on my previously understanding, I
need to return a FuncExpr original even if the function can't be
simplified, so I made a shallow copy. There will be no copy at
all in the following version since I returned NULL in such a case.

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

I misunderstood the _tz flavors return timestamp, after some deep
reading of these functions, they just work at the comparisons part.
so I will add them in the following version.

>
> - 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. :)
>

Yes, I look over my patch carefully before sending it out usually,
but this is an oversight.

Lastly, do you have any idea about the function naming as Jian & I
discussed at [1]?

[1]
https://www.postgresql.org/message-id/CAKU4AWqQ0hed%3DZmpT%2B7Vxnp4H9ZxQqFz30%3Dk%3DvvrmNe57X4dKQ%40mail.gmail.com

--
Best Regards
Andy Fan