On 2019-Jul-03, Amit Langote wrote:
> Hosoya-san,
>
> Thanks for updating the patches.
>
> I have no comment in particular about
> v4_default_partition_pruning.patch,
Cool, thanks. I spent some time reviewing this patch (the first one)
and I propose the attached cosmetic changes. Mostly they consist of a
few comment rewordings.
There is one Assert() that changed in a pretty significant way ...
innocent though the change looks. The original (not Hosoya-san's
patch's fault) had an assertion which is being changed thus:
minoff = 0;
maxoff = boundinfo->ndatums;
...
if (partindices[minoff] < 0)
minoff++;
if (partindices[maxoff] < 0)
maxoff--;
result->scan_default = partition_bound_has_default(boundinfo);
- Assert(minoff >= 0 && maxoff >= 0);
+ Assert(partindices[minoff] >= 0 &&
+ partindices[maxoff] >= 0);
Note that the original Assert() here was verifying whether minoff and
maxoff are both >= 0. But that seems pretty useless since it seems
almost impossible to have them become that given what we do to them.
What I think this code *really* wants to check is whether *the partition
indexes* that they point to are not negative: the transformation that
the two "if" lines do means to ignore bounds that correspond to value
ranges uncovered by any partition. And after the incr/decr operators,
we expect that the bounds will be those of existing partitions ... so
they shouldn't be -1.
Other changes are addition of braces to some one-line blocks that had
significant comments, and minor code rearrangements to make things look
more easily understandable.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
