Re: Report error position in partition bound check

On Thu, 17 Sep 2020 at 13:06, Amit Langote <amitlangote09@gmail.com> wrote:

> Hi Ashutosh,
>
> I had forgotten about this thread, but Michael's ping email brought it
> to my attention.
>
> 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.

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

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

-- 
Best Wishes,
Ashutosh