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