Horton

Re: [HACKERS] [PATCH] Generic type subscripting

Artur Zakirov <a.zakirov@postgrespro.ru>

View thread View on postgr.es
From: Arthur Zakirov <a.zakirov@postgrespro.ru>
To: Dmitry Dolgov <9erthalion6@gmail.com>
Cc: Oleg Bartunov <obartunov@gmail.com>, Peter Eisentraut <peter.eisentraut@2ndquadrant.com>, Tom Lane <tgl@sss.pgh.pa.us>, David Steele <david@pgmasters.net>, PostgreSQL-development <pgsql-hackers@postgresql.org>
Date: 2017-11-16T11:40:17Z
Lists: pgsql-hackers 
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

Commits

  1. Throw error when assigning jsonb scalar instead of a composite object

  2. Filling array gaps during jsonb subscripting

  3. Implementation of subscripting for jsonb

  4. Allow ALTER TYPE to update an existing type's typsubscript value.

  5. Allow subscripting of hstore values.

  6. Support subscripting of arbitrary types, not only arrays.

  7. jit: Reference function pointer types via llvmjit_types.c.

  8. Teach contain_leaked_vars that assignment SubscriptingRefs are leaky.

  9. jit: Correct parameter type for generated expression evaluation functions.

  10. Renaming for new subscripting mechanism

  11. Fix assertion failure for SSL connections.

  12. Teach eval_const_expressions() to handle some more cases.