On Wed, 23 Sep 2020 at 14:41, Amit Langote <amitlangot...@gmail.com> wrote:
> 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. > 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
