(Sorry for leaving this discussion for such a long time, how times fly!)

On Sun, Aug 27, 2023 at 6:28 AM Chapman Flack <chap@anastigmatix.net> wrote:

On 2023-08-22 08:16, Chapman Flack wrote:
> On 2023-08-22 01:54, Andy Fan wrote:
>> After we label it, we will get error like this:
>>
>> select (a->'a')::int4 from m;
>> ERROR: cannot display a value of type internal
>
> Without looking in depth right now, I would double-check
> what relabel node is being applied at the result. The idea,
> of course, was to relabel the result as the expected result

as I suspected, looking at v10-0003, there's this:

+ fexpr = (FuncExpr *)makeRelabelType((Expr *) fexpr, INTERNALOID,
+ 0, InvalidOid,
COERCE_IMPLICIT_CAST);

compared to the example I had sent earlier:

On 2023-08-18 17:02, Chapman Flack wrote:
> expr = (Expr *)makeRelabelType((Expr *)fexpr,
> targetOid, -1, InvalidOid, COERCE_IMPLICIT_CAST);

The key difference: this is the label going on the result type
of the function we are swapping in.

I'm feeling we have some understanding gap in this area, let's

see what it is. Suppose the original query is:

numeric(jsonb_object_field(v_jsonb, text)) -> numeric.

without the patch 003, the rewritten query is:

jsonb_object_field_type(NUMERICOID, v_jsonb, text) -> NUMERIC.

However the declared type of jsonb_object_field_type is:

jsonb_object_field_type(internal, jsonb, text) -> internal.

So the situation is: a). We input a CONST(type=OIDOID, ..) for an

internal argument. b). We return a NUMERIC type which matches

the original query c). result type NUMERIC doesn't match the declared

type 'internal' d). it doesn't match the run-time type of internal

argument which is OID.

case a) is fixed by RelableType. case b) shouldn't be treat as an

issue. I thought you wanted to address the case c), and patch

003 tries to fix it, then the ERROR above. At last I realized case

c) isn't the one you want to fix. case d) shouldn't be requirement

at the first place IIUC.

So your new method is:

1. jsonb_{op}_start() -> internal (internal actually JsonbValue).

2. jsonb_finish_{type}(internal, ..) -> type. (internal is JsonbValue ).

This avoids the case a) at the very beginning. I'd like to provides

patches for both solutions for comparison. Any other benefits of

this method I am missing?

Two more minor differences: (1) the node you get from
makeRelabelType is an Expr, but not really a FuncExpr. Casting
it to FuncExpr is a bit bogus. Also, the third argument to
makeRelabelType is a typmod, and I believe the "not-modified"
typmod is -1, not 0.

My fault, you are right.

On 2023-08-21 06:50, Andy Fan wrote:
> I'm not very excited with this manner, reasons are: a). It will have
> to emit more steps in ExprState->steps which will be harmful for
> execution. The overhead is something I'm not willing to afford.

I would be open to a performance comparison, but offhand I am not
sure whether the overhead of another step or two in an ExprState
is appreciably more than some of the overhead in the present patch,
such as the every-time-through fcinfo initialization buried in
DirectFunctionCall1 where you don't necessarily see it. I bet

the fcinfo in an ExprState step gets set up once, and just has
new argument values slammed into it each time through.

fcinfo initialization in DirectFunctionCall1 is an interesting point!

so I am persuaded the extra steps in ExprState may not be

worse than the current way due to the "every-time-through

fcinfo initialization" (in which case the memory is allocated

once in heap rather than every time in stack). I can do a

comparison at last to see if we can find some other interesting

findings.

I would not underestimate the benefit of reducing the code
duplication and keeping the patch as clear as possible.
The key contributions of the patch are getting a numeric or
boolean efficiently out of the JSON operation. Getting from
numeric to int or float are things the system already does
well.

True, reusing the casting system should be better than hard-code

the casting function manually. I'd apply this on both methods.

A patch that focuses on what it contributes, and avoids
redoing things the system already can do--unless the duplication
can be shown to have a strong performance benefit--is easier to
review and probably to get integrated.

Agreed.

At last, thanks for the great insights and patience!

Best Regards

Andy Fan