Re: [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_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