Re: [PATCH] Generic type subscription

Hello,

On 04.10.2016 11:28, Victor Wagner wrote:
>
> Git complains about whitespace in this version of patch:
>
> $ git apply ../generic_type_subscription_v2.patch
> ../generic_type_subscription_v2.patch:218: tab in indent.
> int first;
> ../generic_type_subscription_v2.patch:219: tab in indent.
> int second;
> ../generic_type_subscription_v2.patch:225: tab in indent.
> SubscriptionExecData *sbsdata = (SubscriptionExecData *) PG_GETARG_POINTER(1);
> ../generic_type_subscription_v2.patch:226: tab in indent.
> Custom *result = (Custom *) sbsdata->containerSource;
> ../generic_type_subscription_v2.patch:234: tab in indent.
> SubscriptionRef *sbsref = (SubscriptionRef *) PG_GETARG_POINTER(0);
> warning: squelched 29 whitespace errors
> warning: 34 lines add whitespace errors.
>
> It doesn't prevent me from further testing of patch, but worth noticing.
>

I agree with Victor. In sgml files whitespaces instead of tabs are
usually used.

I've looked at your patch to make some review.

"subscription"
--------------

The term "subscription" is confusing me. I'm not native english speaker.
But I suppose that this term is not related with the "subscript". So
maybe you should use the "subscripting" or "container" (because you
already use the "container" term in the code). For example:

T_SubscriptingRef
SubscriptingRef
deparseSubscriptingRef()
xsubscripting.sgml
CREATE TYPE custom (
internallength = 4,
input = custom_in,
output = custom_out,
subscripting = custom_subscripting
);
etc.

Subscripting interface
----------------------

In tests I see the query:

> +select ('[1, "2", null]'::jsonb)['1'];
> + jsonb
> +-------
> + "2"
> +(1 row)

Here '1' is used as a second item index. But from the last discussion
https://www.postgresql.org/message-id/55D24517.8080609%40agliodbs.com it
should be a key:

> There is one ambiguous case you need to address:
>
> testjson = '{ "a" : { } }'
>
> SET testjson['a']['a1']['1'] = 42
>
> ... so in this case, is '1' a key, or the first item of an array? how
> do we determine that? How does the user assign something to an array?

And should return null. Is this right? Or this behaviour was not
accepted in discussion? I didn't find it.

> +Datum
> +custom_subscription(PG_FUNCTION_ARGS)
> +{
> + int op_type = PG_GETARG_INT32(0);
> + FunctionCallInfoData target_fcinfo = get_slice_arguments(fcinfo, 1,
> + fcinfo->nargs);
> +
> + if (op_type & SBS_VALIDATION)
> + return custom_subscription_prepare(&target_fcinfo);
> +
> + if (op_type & SBS_EXEC)
> + return custom_subscription_evaluate(&target_fcinfo);
> +
> + elog(ERROR, "incorrect op_type for subscription function: %d", op_type);
> +}

I'm not sure but maybe we should use here two different functions? For
example:

Datum
custom_subscripting_validate(PG_FUNCTION_ARGS)
{
}

Datum
custom_subscripting_evaluate(PG_FUNCTION_ARGS)
{
}

CREATE TYPE custom (
internallength = 8,
input = custom_in,
output = custom_out,
subscripting_validate = custom_subscripting_validate,
subscripting_evalute = custom_subscripting_evaluate,
);

What do you think?

Documentation
-------------

> +<!-- doc/src/sgml/xsubscription.sgml -->
> +
> + <sect1 id="xsubscription">
> + <title>User-defined subscription procedure</title>

There is the extra whitespace before <sect1>.

> + </indexterm>
> + When you define a new base type, you can also specify a custom procedure
> + to handle subscription expressions. It should contains logic for verification
> + and for extraction or update your data. For instance:

I suppose a <para> tag is missed after the </indexterm>.

> +</programlisting>
> +
> + </para>
> +
> + <para>

So </para> is redundant here.

Code
----

> +static Oid
> +findTypeSubscriptionFunction(List *procname, Oid typeOid)
> +{
> + Oid argList[1];

Here typeOid argument is not used. Is it should be here?

> +ExecEvalSubscriptionRef(SubscriptionRefExprState *sbstate,

I think in this function local arguments have a lot of tabs. It is
normal if not all variables is aligned on the same line. A lot of tabs
are occur in other functions too. Because of this some lines exceed 80
characters.

> + if (sbstate->refupperindexpr != NULL)
> + upper = (Datum *) palloc(sbstate->refupperindexpr->length * sizeof(Datum *));
> +
> + if (sbstate->reflowerindexpr != NULL)
> + lower = (Datum *) palloc(sbstate->reflowerindexpr->length * sizeof(Datum *));

Could we use palloc() before the "foreach(l, sbstate->refupperindexpr)"
? I think it could be a little optimization.

> -transformArrayType(Oid *arrayType, int32 *arrayTypmod)
> +transformArrayType(Oid *containerType, int32 *containerTypmod)

I think name of the function is confusing. Maybe use
transformContainerType()?

> +JsonbValue *
> +JsonbToJsonbValue(Jsonb *jsonb, JsonbValue *val)
> +{

In this function we could palloc JsonbValue every time and don't use val
argument. And maybe better to copy all JsonbContainer from jsonb->root
using memcpy(). Instead of assigning reference to jsonb->root. As is the
case in JsonbValueToJsonb().

It is necessary to remove the notice about JsonbToJsonbValue() in
JsonbValueToJsonb() comments.

> -static JsonbValue *
> +JsonbValue *
> setPath(JsonbIterator **it, Datum *path_elems,

Why did you remove static keyword? setPath() is still static.

> - JsonbValue v;
> + JsonbValue v, *new = (JsonbValue *) newval;

Is declaration of "new" variable necessary here? I think it is extra
declaration. Also its name may bring some problems. For example, there
is a thread where guys try to port PostgreSQL to C++.

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