Re: [HACKERS] [PATCH] Generic type subscripting

On Wed, Nov 15, 2017 at 11:02:59PM +0100, Dmitry Dolgov wrote:
> On the second thought, no, looks like I'm wrong and it should be like this.
> The
> reason is that any `fetch` function should be in form
>
> (container, internal) -> extracted value
>
> which means that we need to return an extracted value (for jsonb it's a
> `jsonb`,
> for array it's an `anyelement`). But at the same time in general case we can
> figure out if the result is null only inside a `fetch` function,
> (`jsonb_get_element` for jsonb or whatever it may be for a custom data type)
> because it returns Datum. So the only way to return this information is by
> reference through the `internal` argument. To summarize, If as you said it's
> not that critical, I would suggest to leave it as it is.

Actually it is not only way to return isnull information. You can also return it using pointer to a boolean argument.
*fetch() functions also doesn't need in ExprEvalStep struct, you can pass SubscriptingRefState struct instead.

I mean the following code:

ExecEvalSubscriptingRefFetch(ExprState *state, ExprEvalStep *op)
{
...
*op->resvalue = FunctionCall2(op->d.sbsref.eval_finfo,
PointerGetDatum(*op->resvalue),
PointerGetDatum(op->d.sbsref.state),
PointerGetDatum(op->resnull));
}

Datum
jsonb_subscript_fetch(PG_FUNCTION_ARGS)
{
Datum containerSource = PG_GETARG_DATUM(0);
SubscriptingRefState *state = (SubscriptingRefState *) PG_GETARG_POINTER(1);
bool *isNull = (bool *) PG_GETARG_POINTER(2);

return jsonb_get_element(DatumGetJsonbP(containerSource),
state->upper,
state->numupper,
isNull,
false);
}

> To summarize, If as you said it's
> not that critical, I would suggest to leave it as it is.

Yes, I just wanted to share an opinion how to improve the code. I thought that the current approach may confuse programmers, who will implement subscribting.

Also you can see extractValue() function of GIN [1]. It returns if values is null in same way.

1 - https://www.postgresql.org/docs/current/static/gin-extensibility.html

--
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company