Re: [PATCH] Fix division by zero (explain.c)

On Fri, May 8, 2020 at 7:20 PM Tomas Vondra
<tomas(dot)vondra(at)2ndquadrant(dot)com> wrote:
>
> On Fri, May 08, 2020 at 07:25:36PM -0300, Ranier Vilela wrote:
> >Em sex., 8 de mai. de 2020 às 19:02, Tomas Vondra <
> >tomas(dot)vondra(at)2ndquadrant(dot)com> escreveu:
> >
> >> On Thu, Apr 23, 2020 at 04:12:34PM -0400, James Coleman wrote:
> >> >On Thu, Apr 23, 2020 at 8:38 AM Ranier Vilela <ranier(dot)vf(at)gmail(dot)com>
> >> wrote:
> >> >>
> >> >> Hi,
> >> >>
> >> >> Per Coverity.
> >> >>
> >> >> If has 0 full groups, "we don't need to do anything" and need goes to
> >> next.
> >> >> Otherwise a integer division by zero, can raise.
> >> >>
> >> >> comments extracted trom explain.c:
> >> >> /*
> >> >> * Since we never have any prefix groups unless we've first sorted
> >> >> * a full groups and transitioned modes (copying the tuples into a
> >> >> * prefix group), we don't need to do anything if there were 0 full
> >> >> * groups.
> >> >> */
> >> >
> >> >This does look like a fairly obvious thinko on my part, and the patch
> >> >looks correct to me.
> >> >
> >> >Tomas: agreed?
> >> >
> >>
> >> So how do we actually get the division by zero? It seems to me the fix
> >> prevents a division by zero with 0 full groups and >0 prefix groups,
> >> but can that actually happen?
> >>
> >> But can that actually happen? Doesn't the comment quoted in the report
> >> actually suggest otherwise? If this
> >>
> >> (fullsortGroupInfo->groupCount == 0 &&
> >> prefixsortGroupInfo->groupCount == 0)
> >>
> >
> >> First this line, contradicts the comments. According to the comments,
> >if ( fullsortGroupInfo->groupCount == 0) is true, there is no need to do
> >anything else, next.
> >So anyway, we don't need to test anything anymore.
> >
> >Now, to happen the division by zero, (prefixsortGroupInfo->groupCount == 0,
> >needs to be true too,
> >Maybe this is not happening, but if it happens, it divides by zero, just
> >below, so if an unnecessary test and adds a risk, why not, remove it?
> >
>
> Well, I'd like to understand what the bug is. If possible, I'd like to
> add a test case, for example.
>
> >
> >> evaluates to false, and
> >>
> >> (fullsortGroupInfo->groupCount == 0)
> >>
> >> this evaluates to true, then clearly there would have to be 0 full
> >> groups and >0 prefix groups. But the comment says that can't happen,
> >> unless I misunderstand what it's saying.
> >>
> >Comments says:
> >"we don't need to do anything if there were 0 full groups."
> >
>
> True. But it also implies that in order to have prefix groups we need to
> have a full group first. Which implies that
>
> (#full == 0) && (#prefix != 0)
>
> is not really possible.

There are always full sort groups before any prefix groups can happen,
so we know (even though the tooling doesn't) that the 2nd test can
never contradict the first.

So it's not a bug per se in that we can never reach the place where
the divide by zero would occur, but checking prefix group count
(always 0 if full group count is 0) is confusing at best (and wasn't
intentional), so we should remove it.

James