Re: [PATCH] Generic type subscripting

> On 31 October 2017 at 16:05, Arthur Zakirov <a(dot)zakirov(at)postgrespro(dot)ru>
wrote:
> Documentation is compiled. But there are warnings about end-tags. Now it
is necessary to have full named end-tags

Fixed, thanks for noticing.

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

Yes, I agree. I implemented it in a way that all subscripting-related
functions
must be provided if `subscripting_parse` is there - in this case if you
want to
prevent assign or fetch, you can just do it inside a corresponding function
and
it allows to provide a custom message about that.

> > +Datum
> > +custom_subscripting_parse(PG_FUNCTION_ARGS)
> > +{
> > + bool isAssignment = PG_GETARG_BOOL(0);
>
> Here isAssignment is unused variable, so it could be removed.

In this case I disagree - the purpose of these examples is to show
everything
you can use. So I just need to come up with some example that involves
`isAssignment`.

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

I'm really confused about this one. Can you tell me the exact line numbers?
Because if I remove any of these lines "blindly", tests are failing.

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

Good catch! It was a leftover from the version when I had two different
nodes
for subscripting.

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

Fixed.