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
