MIME-Version: 1.0
Date: Fri, 18 Aug 2023 14:50:15 -0400
From: Chapman Flack <chap@anastigmatix.net>
To: Andy Fan <zhihui.fan1213@gmail.com>
Cc: jian he <jian.universality@gmail.com>, Pavel Stehule
 <pavel.stehule@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>, pgsql-hackers
 <pgsql-hackers@postgresql.org>
Subject: Re: Extract numeric filed in JSONB more effectively
In-Reply-To: <CAKU4AWoLgC5ejOF8jxskd5oq52D-eR_1Q-HM5+e8OBVLak=qTg@mail.gmail.com>
References: <CAKU4AWoqAVya6PBhn+BCbFaBMt3z-2=i5fKO3bW=6HPhbid2Dw@mail.gmail.com>
 <CACJufxHASOEpngQ8V2tbXgs4VZC3ETrVbS=uk0KC_B_J1j7ejQ@mail.gmail.com>
 <CAKU4AWrap1zpYqunJwWTN=CdP7E8e0U4mYmwn7hvTW3ERuENVg@mail.gmail.com>
 <CAFj8pRD-R-GsGCjeYApbhZoiW8TV6zACaYStMBMM0=--+WgN_A@mail.gmail.com>
 <CAKU4AWpDdFXAD+dMC1HeErXSKBUUBRGWkf=dAcX3wZgBNsWM=g@mail.gmail.com>
 <CAFj8pRAO3oEiBaJJ9=HZp6CoP2ffbwSgrKkLKjPfYZwx9wOOuQ@mail.gmail.com>
 <CAKU4AWoCHpKAVuQeOrk44cVPy_dVxn1aHrMUvHy5Ag-daFCSsQ@mail.gmail.com>
 <CAFj8pRD4cdUmK0RG4oN5B2KRSeDhwfMYaL=XpfEu4iaLeZ_Kow@mail.gmail.com>
 <CAKU4AWp8ab61e96v57OaB-Gm1bMfBNVLVy+s17U6_Ne3veB84g@mail.gmail.com>
 <3507485.1691090027@sss.pgh.pa.us>
 <CAKU4AWp+KLes8g=BWLqZfDmW9+=ZY0UC4G0i3qVcYEviK_dDTA@mail.gmail.com>
 <900892914fdc3f477b101d699efb40e0@anastigmatix.net>
 <903341.1692022214@sss.pgh.pa.us>
 <CAKU4AWrBY9GHj9oZbvhiOG1BgiWyZC8FGPAET-CfRKDhYyv1HQ@mail.gmail.com>
 <CAKU4AWrGM5bK7wi4Y8bTYhKgh=A1fW=X00eC_jfk6_JXyaEURQ@mail.gmail.com>
 <CAFj8pRC+4pvSuibB2xcNKJ=6PSF=TAcOtRNpdLPBXZjijFg7ag@mail.gmail.com>
 <CAKU4AWrxHFVZM-gGPpOrVPreZMePAOoY580Tq-+CvxDWHmP_uA@mail.gmail.com>
 <CAKU4AWp3410VpYFdCxpFdaHc8he6zj_=Fvww53TnU+g-bvvvsQ@mail.gmail.com>
 <CACJufxH7ftu9HD+h_gDWPDvq1ZO8vGm81JomSKjvQacCLMLcxg@mail.gmail.com>
 <CAKU4AWqbd_oDwXyK7=yMKbhAR=CQtVOWszcgft+cTG6JCTKmzQ@mail.gmail.com>
 <4b97f1a1dd9b6e45443d24870d3be698@anastigmatix.net>
 <CAKU4AWr1bsGaWWzQHJwB=WXboJrCM242=x6XbHO06vuhWsY4Ww@mail.gmail.com>
 <b2b630ff9804f9eae8205ad721fd0682@anastigmatix.net>
 <CAKU4AWoLgC5ejOF8jxskd5oq52D-eR_1Q-HM5+e8OBVLak=qTg@mail.gmail.com>
User-Agent: Roundcube Webmail/1.6.0
Message-ID: <111272f2dc112c7becdd35ad89f6b935@anastigmatix.net>
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: <https://www.postgresql.org/message-id/111272f2dc112c7becdd35ad89f6b935%40anastigmatix.net>
Precedence: bulk

On 2023-08-18 03:41, Andy Fan wrote:
> I just have
> a quick hack on this, and crash happens at the simplest case.

If I build from this patch, this test:

SELECT (test_json -> 0)::int4, test_json -> 0 FROM test_jsonb WHERE 
json_type = 'scalarint';

fails like this:

Program received signal SIGSEGV, Segmentation fault.
convert_saop_to_hashed_saop_walker (node=0x17, context=0x0)
     at 
/var/tmp/nohome/pgbuildh/../postgresql/src/backend/optimizer/util/clauses.c:2215
2215		if (IsA(node, ScalarArrayOpExpr))

(gdb) p node
$1 = (Node *) 0x17

So the optimizer is looking at some node to see if it is a
ScalarArrayOpExpr, but the node has some rather weird address.

Or maybe it's not that weird. 0x17 is 23, and so is:

select 'int4'::regtype::oid;
  oid
-----
   23

See what happened?

+ int64 target_typ = fexpr->funcresulttype;
...
+ fexpr->args = list_insert_nth(fexpr->args, 0, (void *) target_typ);

This is inserting the desired result type oid directly as the first
thing in the list of fexpr's args.

But at the time your support function is called, nothing is being
evaluated yet. You are just manipulating a tree of expressions to
be evaluated later, and you want fexpr's first arg to be an
expression that will produce this type oid later, when it is
evaluated.

A constant would do nicely:

+ Const	*target  = makeConst(
	INTERNALOID, -1, InvalidOid, SIZEOF_DATUM,
	ObjectIdGetDatum(fexpr->funcresulttype), false, true);
+ fexpr->args = list_insert_nth(fexpr->args, 0, target);

With that change, it doesn't segfault, but it does do this:

ERROR:  cast jsonb to type 0 is not allowed

and that's because of this:

+ Oid			targetOid = DatumGetObjectId(0);

The DatumGetFoo(x) macros are for when you already have the Datum
(it's x) and you know it's a Foo. So this is just setting targetOid
to zero. When you want to get something from function argument 0 and
you know that's a Foo, you use a PG_GETARG_FOO(argno) macro (which
amounts to PG_GETARG_DATUM(argno) followed by DatumGetFoo.

So, with

+ Oid			targetOid = PG_GETARG_OID(0);

SELECT (test_json -> 0)::int4, test_json -> 0 FROM test_jsonb WHERE 
json_type = 'scalarint';
  int4 | ?column?
------+----------
     2 | 2

However, EXPLAIN is sad:

ERROR:  cannot display a value of type internal

and that may be where this idea runs aground.

Now, I was expecting something to complain about the result of
jsonb_array_element_type, and that didn't happen. We rewrote
a function that was supposed to be a cast to int4, and
replaced it with a function returning internal, and evaluation
happily just took that as the int4 that the next node expected.

If something had complained about that, it might have been
necessary to insert some new node above the internal-returning
function to say the result was really int4. Notice there is a
makeRelabelType() for that. (I had figured there probably was,
but didn't know its exact name.)

So it doesn't seem strictly necessary to do that, but it might
make the EXPLAIN result look better (if EXPLAIN were made to work,
of course).

Now, my guess is EXPLAIN is complaining when it sees the Const
of type internal, and doesn't know how to show that value.
Perhaps makeRelabelType is the answer there, too: what if the
Const has Oid type, so EXPLAIN can show it, and what's inserted
as the function argument is a relabel node saying it's internal?

Haven't tried that yet.

Regards,
-Chap