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

On Fri, May 08, 2020 at 07:33:03PM -0400, James Coleman wrote:
>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.
>

OK, thanks for the clarification.

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services