Re: Report error position in partition bound check

On Wed, 23 Sep 2020 at 14:41, Amit Langote <amitlangote09(at)gmail(dot)com> wrote:

> Thanks Ashutosh.
>
> On Fri, Sep 18, 2020 at 7:33 PM Ashutosh Bapat
> <ashutosh(dot)bapat(at)2ndquadrant(dot)com> wrote:
> > Thanks Amit for addressing comments.
> >
> > @@ -4256,5 +4256,8 @@ transformPartitionBoundValue(ParseState *pstate,
> Node *val,
> > if (!IsA(value, Const))
> > elog(ERROR, "could not evaluate partition bound expression");
> >
> > + /* Preserve parser location information. */
> > + ((Const *) value)->location = exprLocation(val);
> > +
> > return (Const *) value;
> > }
> >
> > This caught my attention and I was wondering whether transformExpr()
> itself should transfer the location from input expression to the output
> expression. Some minions of transformExprRecurse() seem to be doing that.
> The change here may be an indication that some of them are not doing this.
> In that case may be it's better to find those and fix rather than a
> white-wash fix here. In what case did we find that location was not set by
> transformExpr? Sorry for not catching this earlier.
>
> AFAICS, transformExpr() is fine. What loses the location value is the
> unconditional evaluate_expr() call which generates a fresh Const node,
> possibly after evaluating a non-Const expression that is passed to it.
> I don't find it very desirable to change evaluate_expr() to accept a
> location value, because other callers of it don't seem to care.
> Instead, in the updated patch, I have made calling evaluate_expr()
> conditional on the expression at hand being a non-Const node and
> assign location by hand on return. If the expression is already
> Const, we don't need to update the location field as it should already
> be correct. Though, I did notice that the evaluate_expr() call has an
> additional responsibility which is to pass the partition key specified
> collation to the bound expression, so we should not fail to update an
> already-Const node's collation likewise.
>

Thanks for the detailed explanation. I am not sure whether skipping one
evaluate_expr() call for a constant is better or reassigning the location.
This looks better than the last patch.

> > /* New lower bound is certainly >= bound at offet. */
> > offet/offset? But this comment is implied by the comment just two lines
> above. So I am not sure it's really needed.
>
> Given that cmpval is set all the way in partition_range_bsearch(), I
> thought it would help to clarify why this code can assume it must be
> >= 0. It is because a valid offset returned by
> partition_range_bsearch() must correspond to a bound that it found to
> be <= the probe bound passed to it.
>

> > /* Fetch the problem bound from lower datums list. */
> > This is fetching problematic key value rather than the whole problematic
> bound. I think the comment would be useful if it explains why cmpval -1 th
> key is problematic but then that's evident from the prologue of
> partition_rbound_cmp() so I am not sure if this comment is really required.
> For example, we aren't adding a comment here
> > + overlap_location = ((PartitionRangeDatum *)
> > + list_nth(spec->upperdatums, -cmpval - 1))->location;
>
> In the attached updated patch, I have tried to make the code and
> comments for different cases consistent. Please have a look.
>
>

The comments look okay to me. I don't see a way to keep them short and yet
avoid reading the prologue of partition_range_bsearch(). And there is no
point in repeating a portion of that prologue at multiple places. So I am
fine with these set of comments.

Setting this CF entry as "RFC". Thanks.

--
Best Wishes,
Ashutosh