Re: Extract numeric filed in JSONB more effectively

On 2023-08-17 21:14, Andy Fan wrote:
>> The idea of an 'internal' return type with no 'internal' parameter
>> was quickly and rightly shot down.
> 
> Yes, it mainly breaks the type-safety system.  Parser need to know
> the result type, so PG defines the rule like this:

Well, the reason "internal return type with no internal parameter type"
was shot down was more specific: if there is such a function in the
catalog, an SQL user can call it, and then its return type is a value
typed 'internal', and with that the SQL user could call other
functions with 'internal' parameters, and that's what breaks type
safety. The specific problem is not having at least one 'internal'
input parameter.

There are lots of functions in the catalog with internal return type
(I count 111). They are not inherently bad; the rule is simply that
each one also needs at least one IN parameter typed internal, to
make sure it can't be directly called from SQL.

> anyelement fn(anyment in);
> 
> if the exprType(in) in the query tree is X, then PG would think fn
> return type X.  that's why we have to have an anyelement in the
> input.

That's a consequence of the choice to have anyelement as the return
type, though. A different choice wouldn't have that consequence.

> I have some trouble understanding this.  are you saying something
> like:
> 
> internal fn(internal jsonValue,  internal typeOid)?
> 
> If so, would it break the type-safety system?

That is what I'm saying, and it doesn't break type safety at the
SQL level, because as long as it has parameters declared internal,
no SQL can ever call it. So it can only appear in an expression
tree because your SupportRequestSimplify put it there properly
typed, after the SQL query was parsed but before evaluation.

The thing about 'internal' is it doesn't represent any specific
type, it doesn't necessarily represent the same type every time it
is mentioned, and it often means something that isn't a cataloged
type at all, such as a pointer to some kind of struct. There must be
documentation explaining what it has to be. For example, your
jsonb_cast_support function has an 'internal' parameter and
'internal' return type. From the specification for support
functions, you know the 'internal' for the parameter type means
"one of the Node structs in supportnodes.h", and the 'internal'
for the return type means "an expression tree semantically
equivalent to the FuncExpr".

So, in addition to declaring
internal fn(internal jsonValue,  internal typeOid), you would
have to write a clear spec that jsonValue has to be a JsonbValue,
typeOid has to be something you can call DatumGetObjectId on,
and the return value should be a Datum in proper form
corresponding to typeOid. And, of course, generate the expression
tree so all of that is true when it's evaluated.

>> Perhaps there are parts of that rewriting that no existing node type
>> can represent?

The description above was in broad strokes. Because I haven't
tried to implement this, I don't know whether some roadblock would
appear, such as, is it hard to make a Const node of type internal
and containing an oid? Or, what sort of node must be inserted to
clarify that the 'internal' return is actually a Datum of the
expected type? By construction, we know that it is, but how to
make that explicit in the expression tree?

> a).  we can't use the makeNullConst because jsonb_xxx_type is
> strict,  so if we have NULL constant input here,  the PG system
> will return NULL directly.  b).  Not only the type oid is the thing
> We are interested in the  const.constvalue is as well since
> 'explain select xxxx'  to access it to show it as a string.
> Datum(0) as the constvalue will crash in this sense.  That's why
> makeDummyConst was introduced.

Again, all of that complication stems from the choice to use the
anyelement return type and rely on polymorphic type resolution
to figure the oid out, when we already have the oid to begin with
and the oid is all we want.

>> something like "assertion of
>> 'internal'-to-foo binary coercibility, vouched by a prosupport
>> function", would that be a bad thing?
> 
> I can't follow this as well.

That was just another way of saying what I was getting at above
about what's needed in the expression tree to indicate that the
'internal' produced by this function is, in fact, really a bool
(or whatever). We know that it is, but perhaps the expression
tree will be considered ill-formed without a node that says so.
A node representing a no-op, binary conversion would suffice,
but is there already a node that's allowed to represent an
internal-to-bool no-op cast?

If there isn't, one might have to be invented. So it might be that
if we go down the "use polymorphic resolution" road, we have to
invent dummy Consts, and down the "internal" road we also have to
invent something, like the "no-op cast considered correct because
a SupportRequestSimplify function put it here" node.

If it came down to having to invent one of those things or the
other, I'd think the latter more directly captures what we really
want to do.

(And I'm not even sure anything has to be invented. If there's an
existing node for no-op binary casts, I think I'd first try
putting that there and see if anything complains.)

If this thread is being followed by others more familiar with
the relevant code or who see obvious problems I'm missing,
please chime in!

Regards,
-Chap