Thanks Ashutosh. On Fri, Sep 18, 2020 at 7:33 PM Ashutosh Bapat <ashutosh.ba...@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. -- Amit Langote EnterpriseDB: http://www.enterprisedb.com
v5-0001-Improve-check-new-partition-bound-error-position-.patch
Description: Binary data
