Re: [PATCH] Generic type subscription - Mailing list pgsql-hackers

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