On Sun, Oct 29, 2017 at 10:56:19PM +0100, Dmitry Dolgov wrote:
> 
> So, here is the new version of patch that contains modifications we've
> discussed, namely:
> 
> * store oids of `parse`, `fetch` and `assign` functions
> 
> * introduce dependencies from a data type
> 
> * as a side effect of previous two I also eliminated some unnecessary
> arguments
>   in `parse` function.

Thank you for new version of the patch.

There are some notes.

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

Documentation is compiled. But there are warnings about end-tags. Now it is 
necessary to have full named end-tags:

=# make -C doc/src/sgml check
/usr/sbin/onsgmls:json.sgml:574:20:W: empty end-tag
...

Documentation is out of date:
- catalogs.sgml needs to add information about additional pg_type fields
- create_type.sgml needs information about subscripting_parse, 
subscripting_assign and subscripting_fetch options
- xsubscripting.sgml is out of date

Code
----

I think it is necessary to check Oids of subscripting_parse, 
subscripting_assign, subscripting_fetch. Maybe within TypeCreate().

Otherwise next cases possible:

=# CREATE TYPE custom (
   internallength = 8,
   input = custom_in,
   output = custom_out,
   subscripting_parse = custom_subscripting_parse);
=# CREATE TYPE custom (
   internallength = 8,
   input = custom_in,
   output = custom_out,
   subscripting_fetch = custom_subscripting_fetch);

Are all subscripting_* fields mandatory? If so if user provided at least one of 
them then all fields should be provided.

Should all types have support assigning via subscript? If not then 
subscripting_assign parameter is optional.

> +Datum
> +jsonb_subscript_parse(PG_FUNCTION_ARGS)
> +{
> +     bool                            isAssignment = PG_GETARG_BOOL(0);

and

> +Datum
> +custom_subscripting_parse(PG_FUNCTION_ARGS)
> +{
> +     bool                            isAssignment = PG_GETARG_BOOL(0);

Here isAssignment is unused variable, so it could be removed.

> +
> +     scratch->d.sbsref.eval_finfo = eval_finfo;
> +     scratch->d.sbsref.nested_finfo = nested_finfo;
> +

As I mentioned earlier we need assigning eval_finfo and nested_finfo only for 
EEOP_SBSREF_OLD, EEOP_SBSREF_ASSIGN and EEOP_SBSREF_FETCH steps.
Also they should be assigned before calling ExprEvalPushStep(), not after. 
Otherwise some bugs may appear in future.

> -             ArrayRef   *aref = makeNode(ArrayRef);
> +             NodeTag sbstag = nodeTag(src_expr);
> +             Size nodeSize = sizeof(SubscriptingRef);
> +             SubscriptingRef *sbsref = (SubscriptingRef *) newNode(nodeSize, 
> sbstag);

Is there necessity to use newNode() instead using makeNode(). The previous code 
was shorter.

There is no changes in execnodes.h except removed line. So I think execnodes.h 
could be removed from the patch.

> 
> I'm going to make few more improvements, but in the meantime I hope we can
> continue to review the patch.

I will wait.

-- 
Arthur 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:
http://www.postgresql.org/mailpref/pgsql-hackers

Reply via email to