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
diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c
index c275387319..42a0d31c31 100644
--- a/src/backend/executor/execExpr.c
+++ b/src/backend/executor/execExpr.c
@@ -2350,9 +2350,6 @@ ExecInitSubscriptingRef(ExprEvalStep *scratch, SubscriptingRef *aref, PlanState
fmgr_info(aref->refnestedfunc, nested_finfo);
}
- scratch->d.sbsref.eval_finfo = eval_finfo;
- scratch->d.sbsref.nested_finfo = nested_finfo;
-
/* Fill constant fields of SubscriptingRefState */
arefstate->isassignment = isAssignment;
arefstate->refelemtype = aref->refelemtype;
@@ -2377,9 +2374,6 @@ ExecInitSubscriptingRef(ExprEvalStep *scratch, SubscriptingRef *aref, PlanState
scratch->d.jump.jumpdone = -1; /* adjust later */
ExprEvalPushStep(state, scratch);
- scratch->d.sbsref.eval_finfo = eval_finfo;
- scratch->d.sbsref.nested_finfo = nested_finfo;
-
adjust_jumps = lappend_int(adjust_jumps,
state->steps_len - 1);
}
@@ -2425,9 +2419,6 @@ ExecInitSubscriptingRef(ExprEvalStep *scratch, SubscriptingRef *aref, PlanState
scratch->d.sbsref_subscript.jumpdone = -1; /* adjust later */
ExprEvalPushStep(state, scratch);
- scratch->d.sbsref.eval_finfo = eval_finfo;
- scratch->d.sbsref.nested_finfo = nested_finfo;
-
adjust_jumps = lappend_int(adjust_jumps,
state->steps_len - 1);
i++;
@@ -2462,9 +2453,6 @@ ExecInitSubscriptingRef(ExprEvalStep *scratch, SubscriptingRef *aref, PlanState
scratch->d.sbsref_subscript.jumpdone = -1; /* adjust later */
ExprEvalPushStep(state, scratch);
- scratch->d.sbsref.eval_finfo = eval_finfo;
- scratch->d.sbsref.nested_finfo = nested_finfo;
-
adjust_jumps = lappend_int(adjust_jumps,
state->steps_len - 1);
i++;
@@ -2499,10 +2487,9 @@ ExecInitSubscriptingRef(ExprEvalStep *scratch, SubscriptingRef *aref, PlanState
{
scratch->opcode = EEOP_SBSREF_OLD;
scratch->d.sbsref.state = arefstate;
- ExprEvalPushStep(state, scratch);
-
scratch->d.sbsref.eval_finfo = eval_finfo;
scratch->d.sbsref.nested_finfo = nested_finfo;
+ ExprEvalPushStep(state, scratch);
}
/* SBSREF_OLD puts extracted value into prevvalue/prevnull */
@@ -2521,10 +2508,9 @@ ExecInitSubscriptingRef(ExprEvalStep *scratch, SubscriptingRef *aref, PlanState
/* and perform the assignment */
scratch->opcode = EEOP_SBSREF_ASSIGN;
scratch->d.sbsref.state = arefstate;
- ExprEvalPushStep(state, scratch);
-
scratch->d.sbsref.eval_finfo = eval_finfo;
scratch->d.sbsref.nested_finfo = nested_finfo;
+ ExprEvalPushStep(state, scratch);
}
else
@@ -2532,10 +2518,9 @@ ExecInitSubscriptingRef(ExprEvalStep *scratch, SubscriptingRef *aref, PlanState
/* array fetch is much simpler */
scratch->opcode = EEOP_SBSREF_FETCH;
scratch->d.sbsref.state = arefstate;
- ExprEvalPushStep(state, scratch);
-
scratch->d.sbsref.eval_finfo = eval_finfo;
scratch->d.sbsref.nested_finfo = nested_finfo;
+ ExprEvalPushStep(state, scratch);
}
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
