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

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.vondra@2ndquadrant.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.vf@gmail.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.


regards

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