On 2018/07/27 12:14, Tom Lane wrote:
> David Rowley <david.rowley@2ndquadrant.com> writes:
>> On 27 July 2018 at 13:35, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>>> On 2018/07/27 1:28, Tom Lane wrote:
>>>> (BTW, I'm not sure that it was wise to design bms_add_range to fail for
>>>> empty ranges. Maybe it'd be better to redefine it as a no-op for
>>>> upper < lower?)
>
>>> FWIW, I was thankful that David those left those checks there, because it
>>> helped expose quite a few bugs when writing this code or perhaps that was
>>> his intention to begin with, but maybe he thinks differently now (?).
>
>> I think it's more useful to keep as a bug catcher, although I do
>> understand the thinking behind just having it be a no-op.
>
> Well, my thinking is that it helps nobody if call sites have to have
> explicit workarounds for a totally-arbitrary refusal to handle edge
> cases in the primitive functions. I do not think that is good software
> design. If you want to have assertions that particular call sites are
> specifying nonempty ranges, put those in the call sites where it's
> important. But as-is, this seems like, say, defining foreach() to
> blow up on an empty list.
How about adding Asserts to that effect in partprune.c and execPartition.c
where they're not present? These are the only two files where it's
currently being used, given that it was invented only recently and exactly
for use by the new pruning code. I updated the patch that I posted in the
last email to add those Asserts.
FWIW, I can see at least the guard against < 0 values in number of other
bms_* functions, but I won't be the one to make a call one way or the
other about whether to change bms_add_range() to cope with erroneous inputs.
Thanks,
Amit
