Re: [PATCH] Generic type subscripting

Thank you for fixing.

On Tue, Nov 07, 2017 at 09:00:43PM +0100, Dmitry Dolgov wrote:
> > > +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`.

Understood.

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

Field scratch->d is union. Its fields should be changed only before calling ExprEvalPushStep(), which copies 'scratch'. To be more specific I attached the patch 0005-Fix-ExprEvalStep.patch, which can be applyed over your patches.

Some other notes are below.

> <replaceable class="parameter">type_modifier_output_function</replaceable> and
> - <replaceable class="parameter">analyze_function</replaceable>
> + <replaceable class="parameter">analyze_function</replaceable>,
> + <replaceable class="parameter">subscripting_parse_function</replaceable>
> + <replaceable class="parameter">subscripting_assign_function</replaceable>
> + <replaceable class="parameter">subscripting_fetch_function</replaceable>

I think you forgot commas and conjunction 'and'.

> + The optional
> + <replaceable class="parameter">subscripting_parse_function</replaceable>,
> + <replaceable class="parameter">subscripting_assign_function</replaceable>
> + <replaceable class="parameter">subscripting_fetch_function</replaceable>
> + contains type-specific logic for subscripting of the data type.

Here you forgot comma or 'and'. Also 'contain' should be used instead 'contains'.

It seems that in the following you switched descriptions:

> + <term><replaceable class="parameter">subscripting_assign_function</replaceable></term>
> + <listitem>
> + <para>
> + The name of a function that contains type-specific subscripting logic for
> + fetching an element from the data type.
> + </para>

subscripting_assign_function is used for updating.

> + <term><replaceable class="parameter">subscripting_fetch_function</replaceable></term>
> + <listitem>
> + <para>
> + The name of a function that contains type-specific subscripting logic for
> + updating an element in the data type.
> + </para>

subscripting_fetch_function is used for fetching.

I have a little complain about how ExprEvalStep gets resvalue. resvalue is assigned in one place (within ExecEvalSubscriptingRefFetch(), ExecEvalSubscriptingRefAssign()), resnull is assigned in another place (within jsonb_subscript_fetch(), jsonb_subscript_assign()). I'm not sure that it is a good idea, but it is not critical, it is just complaint.

After your fixing I think we should wait for opinion of senior community members and mark the patch as 'Ready for Commiter'. Maybe I will do more tests and try to implement subscripting to another type.

--
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company