Re: [HACKERS] [PATCH] Generic type subscripting

> On 31 October 2017 at 16:05, Arthur Zakirov <a.zakirov@postgrespro.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.