Subject: Re: Extract numeric filed in JSONB more effectively

From: Andy Fan <[email-masked]>
Date: Mon, 14 Aug 2023 15:06:30 +0800

>
>
> We'd still have functions like jsonb_field_as_numeric() under the
> hood, but there's not an expectation that users call them explicitly.
>

To avoid the lots of functions like jsonb_field_as_int2/int4, I defined
Datum jsonb_object_field_type(.., Oid target_oid) at last,  so the
function must return "internal" or "anyelement".  Then we can see:

select jsonb_object_field_type(tb.a, 'a'::text, 1700) from tb;
ERROR:  cannot display a value of type anyelement.

The reason is clear to me, but  I'm not sure how to fix that or deserves
a fix? Or shall I define jsonb_object_field_int2/int8 to avoid this?

This is an unresolved issue at the latest patch.
-- 
Best Regards
Andy Fan

---

From: Pavel Stehule <[email-masked]>
Date: Mon, 14 Aug 2023 10:01:31 +0200

> ERROR:  cannot display a value of type anyelement.

you cannot to use type as parameter. There should be some typed value - like

jsonb_object_field, '{"a":10}', 'a', NULL::int)

and return type should be anyelement.

Another solution should be more deeper change like implementation of
"coalesce"

---

From: Chapman Flack <[email-masked]>
Date: Mon, 14 Aug 2023 10:04:03 -0400

On 2023-08-14 03:06, Andy Fan wrote:
> function must return "internal" or "anyelement".
> ...
> I'm not sure how to fix that or deserves

As far as I'm concerned, if the intent is for this to be a function
that is swapped in by SupportRequestSimplify and not necessarily to
be called by users directly, I don't mind if users can't call it
directly. As long as there is a nice familiar jsonb function the user
can call in a nice familiar way and knows it will be handled
efficiently behind the curtain, that seems to be good enough for
the user--better, even, than having a new oddball function to
remember.

However, I believe the rule is that a function declared to return
internal must also declare at least one parameter as internal.
That way, a user won't be shown errors about displaying its
returned value, because the user won't be able to call it
in the first place, having no values of type 'internal' lying
around to pass to it. It could simply have that trailing oid
parameter declared as internal, and there you have a strictly
internal-use function.

Providing a function with return type declared internal but
with no parameter of that type is not good, because then a
user could, in principle, call it and obtain a value of
'internal' type, and so get around the typing rules that
prevent calling other internal functions.

Regards,
-Chap

---

From: Tom Lane <[email-masked]>
Date: Mon, 14 Aug 2023 10:10:14 -0400

Chapman Flack <[email-masked]> writes:
> with no parameter of that type is not good,

Not so much "not good" as "absolutely, positively WILL NOT HAPPEN".

> because then a

Right --- it'd completely break the system's type-safety for
other internal-using functions.

You could argue that we should never have abused "internal"
to this extent in the first place, compared to inventing a
plethora of internal-ish types to correspond to each of the
things "internal" is used for.  But here we are so we'd
better be darn careful with it.

			regards, tom lane

---

From: Andy Fan <[email-masked]>
Date: Mon, 14 Aug 2023 23:42:12 +0800

> Not so much "not good" as "absolutely, positively WILL NOT HAPPEN".

Chap is pretty nice to others:).

> other internal-using functions.
I do see something bad in opr_sanity.sql.  Pavel suggested
get_fn_expr_argtype which can resolve this issue pretty well, so
I have changed

jsonb_extract_xx_type(..,  Oid taget_oid) -> anyelement.
to
jsonb_extract_xx_type(..,  anyelement) -> anyelement.

The only bad smell left is since I want to define jsonb_extract_xx_type
as strict so I can't use   jsonb_extract_xx_type(.., NULL::a-type)
since it will be evaluated to NULL directly.  So I hacked it with

/* mock the type. */
            Const   *target =  makeNullConst(fexpr->funcresulttype,
                                             -1,
                                             InvalidOid);

/* hack the NULL attribute */
            /*

             * Since all the above functions are strict, we can't input

             * a NULL value.

             */
            target->constisnull = false;

 jsonb_extract_xx_type just cares about the argtype, but
'explain select xx'  will still access the const->constvalue.
const->constvalue is 0 which is set by makeNullConst currently,
and it is ok for the current supported type. but I'm not sure
about the future or if we still have a better solution.

v6 is attached.  any feedback is welcome!

-- 
Best Regards
Andy Fan

---

From: Andy Fan <[email-masked]>
Date: Tue, 15 Aug 2023 11:24:35 +0800

> and it is ok for the current supported type.
>

The exception is numeric data type, the constvalue can't be 0.
so hack it with the below line.  maybe not good enough,  but I
have no better solution now.

+                       Const   *target =
 makeNullConst(fexpr->funcresulttype,
+
             -1,
+
             InvalidOid);
+                       /*
+                        * Since all the above functions are strict, we
can't input
+                        * a NULL value.
+                        */
+                       target->constisnull = false;
+
+                       Assert(target->constbyval || target->consttype ==
NUMERICOID);
+
+                       /* Mock a valid datum for !constbyval type. */
+                       if (fexpr->funcresulttype == NUMERICOID)
+                               target->constvalue =
DirectFunctionCall1(numeric_in, CStringGetDatum("0"));

-- 
Best Regards
Andy Fan

---

From: Pavel Stehule <[email-masked]>
Date: Tue, 15 Aug 2023 07:23:26 +0200

Hi

> DirectFunctionCall1(numeric_in, CStringGetDatum("0"));
Personally I think this workaround is too dirty, and better to use a strict
function (I believe so the overhead for NULL values is acceptable), or
introduce a different mechanism.

Your design is workable, and I think acceptable, but I don't think it is an
ideal or final solution. It is not really generic. It doesn't help with XML
or Hstore. You need to touch cast functions, which I think is not best,
because cast functions should not cooperate on optimization of execution of
another function.

My idea of an ideal solution is the introduction of the possibility to use
"any" pseudotype as return type with possibility to set default return
type. Now, "any" is allowed only for arguments. The planner can set the
expected type when it knows it, or can use the default type.

so for extraction of jsonb field we can use FUNCTION
jsonb_extract_field(jsonb, text) RETURNS "any" DEFAULT jsonb

if we call SELECT jsonb_extract_field(..., 'x') -> then it returns jsonb,
if we use SELECT jsonb_extract_field('...', 'x')::date, then it returns date

With this possibility we don't need to touch to cast functions, and we can
simply implement similar functions for other non atomic types.

--

---

From: Pavel Stehule <[email-masked]>
Date: Tue, 15 Aug 2023 07:33:03 +0200

út 15. 8. 2023 v 7:23 odesílatel Pavel Stehule <[email-masked]>
napsal:

> Personally I think this workaround is too dirty, and better to use a
> strict function (I believe so the overhead for NULL values is acceptable),
> or introduce a different mechanism.
>
> Your design is workable, and I think acceptable, but I don't think it is
> an ideal or final solution. It is not really generic. It doesn't help with
> XML or Hstore. You need to touch cast functions, which I think is not best,

this syntax can be used instead NULL::type trick

like

SELECT jsonb_populate_record('{...}')::pg_class;

instead

SELECT jsonb_populate_record(NULL::pg_class, '{...}')

---

From: Andy Fan <[email-masked]>
Date: Tue, 15 Aug 2023 14:04:19 +0800

> jsonb_extract_field(jsonb, text) RETURNS "any" DEFAULT jsonb

Is this an existing framework or do you want to create something new?

> if we use SELECT jsonb_extract_field('...', 'x')::date, then it returns date

If so, what is the difference from the current  jsonb->'f'   and
(jsonb->'f' )::date?

> simply implement similar functions for other non atomic types.

What do you mean by "atomic type" here?   If you want to introduce some new
framework,  I think we need a very clear benefit.

-- 
Best Regards
Andy Fan

---

From: Pavel Stehule <[email-masked]>
Date: Tue, 15 Aug 2023 08:50:25 +0200

út 15. 8. 2023 v 8:04 odesílatel Andy Fan <[email-masked]> napsal:

>
>> My idea of an ideal solution is the introduction of the possibility to
>> use "any" pseudotype as return type with possibility to set default return

This should be created

> If so, what is the difference from the current  jsonb->'f'   and

a) effectiveness. The ending performance should be similar like your
current patch, but without necessity to use planner support API.

b) more generic usage. For example, the expressions in plpgsql are executed
a little bit differently than SQL queries. So there the optimization from
your patch probably should not work, because you can write only var :=
j->'f', and plpgsql forces cast function execution, but not via planner.

c) nothing else. It should not to require to modify cast function
definitions

>> With this possibility we don't need to touch to cast functions, and we
>> can simply implement similar functions for other non atomic types.
>>
>
> What do you mean by "atomic type" here?   If you want to introduce some
> new framework,  I think we need a very clear benefit.
>

Atomic types (skalar types like int, varchar, date), nonatomic types -
array, composite, xml, jsonb, hstore or arrays of composite types.

---

From: Andy Fan <[email-masked]>
Date: Tue, 15 Aug 2023 15:05:26 +0800

> current patch, but without necessity to use planner support API.

So the cost is we need to create a new & different framework.

b) because you can write only var := j->'f', and plpgsql forces cast
> function execution, but not via planner.
>

var a := 1 needs going with planner,  IIUC,  same with j->'f'.

> definitions

If you look at how the planner support function works,  that is
pretty simple,  just modify the prosupport attribute. I'm not sure
this should be called an issue or avoiding it can be described
as a benefit.

I don't think the current case is as bad as the other ones like
users needing to modify their queries or type-safety system
being broken. So personally I'm not willing to creating some
thing new & heavy. However I'm open to see what others say.

-- 
Best Regards
Andy Fan

---

From: Pavel Stehule <[email-masked]>
Date: Tue, 15 Aug 2023 09:45:59 +0200

> So the cost is we need to create a new & different framework.

yes, it can be less work, code than for example introduction of
"anycompatible".

> var a := 1 needs going with planner,  IIUC,  same with j->'f'.

i was wrong, the planner is full, but the executor is reduced.

> thing new & heavy. However I'm open to see what others say.

ok

regards

Pavel

---

From: Andy Fan <[email-masked]>
Date: Wed, 16 Aug 2023 14:12:16 +0800

On Tue, Aug 15, 2023 at 1:24 PM Pavel Stehule <[email-masked]>
wrote:

> strict function (I believe so the overhead for NULL values is acceptable).
>

In the patch v8,  I created a new routine named makeDummyConst,
which just sits by makeNullConst. It may be helpful to some extent.
a).  The code is self-document for the user/reader.  b).  We have a
central place to maintain this routine.

Besides the framework,  the troubles for the reviewer may be if the
code has some corner case issue or behavior changes. Especially
I have some code refactor when working on jsonb_extract_path.
so the attached test.sql is designed for this.  I have compared the
result between master and patched version and I think reviewer
can do some extra testing with it.

v8 is the finished version in my mind, so I think it is ready for review
now.

-- 
Best Regards
Andy Fan

---

From: Andy Fan <[email-masked]>
Date: Wed, 16 Aug 2023 14:27:57 +0800

update with the correct patch..