Re: [HACKERS] [PATCH] Generic type subscripting

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_FETCHsteps.
 
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