Re: Report error position in partition bound check - Mailing list pgsql-hackers

Thanks Ashutosh.

On Fri, Sep 18, 2020 at 7:33 PM Ashutosh Bapat
<ashutosh.bapat@2ndquadrant.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.

> /* 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.