Re: [HACKERS] [PATCH] Generic type subscripting - Mailing list pgsql-hackers

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'.
Tobe 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
justcomplaint.
 

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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers