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 *) 
../generic_type_subscription_v2.patch:226: tab in indent.
        Custom                                          *result = (Custom *) 
../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.


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:

CREATE TYPE custom (
   internallength = 4,
   input = custom_in,
   output = custom_out,
   subscripting = custom_subscripting

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.

+       int                                             op_type = 
+       FunctionCallInfoData    target_fcinfo = get_slice_arguments(fcinfo, 1,
+       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:



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?


+<!-- 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>.

+ </para>
+  <para>

So </para> is redundant here.


+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

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to