On Thu, 17 Sep 2020 at 13:06, Amit Langote <amitlangot...@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
