Обсуждение: [PATCH] Fix division by zero (explain.c)

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?

Thanks,
James

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)

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.

I've tried to trigger the issue, but without success ...


regards

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

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

On Fri, May 8, 2020 at 7:20 PM Tomas Vondra
<tomas.vondra@2ndquadrant.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.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.

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

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

James Coleman <jtc331@gmail.com> writes:
> 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 maybe an assertion enforcing that would be appropriate?
Untested, but:

-            if (fullsortGroupInfo->groupCount == 0 &&
-                prefixsortGroupInfo->groupCount == 0)
+            if (fullsortGroupInfo->groupCount == 0)
+            {
+                Assert(prefixsortGroupInfo->groupCount == 0);
                 continue;
+            }

            regards, tom lane

On Sat, May 09, 2020 at 06:48:59AM -0300, Ranier Vilela wrote:
>Em sáb., 9 de mai. de 2020 às 01:45, Tom Lane <tgl@sss.pgh.pa.us> escreveu:
>
>> James Coleman <jtc331@gmail.com> writes:
>> > 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 maybe an assertion enforcing that would be appropriate?
>> Untested, but:
>>
>> -                       if (fullsortGroupInfo->groupCount == 0 &&
>> -                               prefixsortGroupInfo->groupCount == 0)
>> +                       if (fullsortGroupInfo->groupCount == 0)
>> +                       {
>> +                               Assert(prefixsortGroupInfo->groupCount ==
>> 0);
>>                                 continue;
>> +                       }
>>
>I agree, asserts always help.
>

That doesn't work, because the prefixSortGroupInfo is used before
assignment, producing compile-time warnings.

I've pushed a simpler fix without the assert. If we want to make this
check, perhaps doing it in incremental sort itself would be better than
doing it in explain.

regards

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

On Sat, May 09, 2020 at 02:51:50PM -0300, Ranier Vilela wrote:
>Em sáb., 9 de mai. de 2020 às 14:44, Tomas Vondra <
>tomas.vondra@2ndquadrant.com> escreveu:
>
>> On Sat, May 09, 2020 at 06:48:59AM -0300, Ranier Vilela wrote:
>> >Em sáb., 9 de mai. de 2020 às 01:45, Tom Lane <tgl@sss.pgh.pa.us>
>> escreveu:
>> >
>> >> James Coleman <jtc331@gmail.com> writes:
>> >> > 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 maybe an assertion enforcing that would be appropriate?
>> >> Untested, but:
>> >>
>> >> -                       if (fullsortGroupInfo->groupCount == 0 &&
>> >> -                               prefixsortGroupInfo->groupCount == 0)
>> >> +                       if (fullsortGroupInfo->groupCount == 0)
>> >> +                       {
>> >> +                               Assert(prefixsortGroupInfo->groupCount
>> ==
>> >> 0);
>> >>                                 continue;
>> >> +                       }
>> >>
>> >I agree, asserts always help.
>> >
>>
>> That doesn't work, because the prefixSortGroupInfo is used before
>> assignment, producing compile-time warnings.
>>
>> I've pushed a simpler fix without the assert. If we want to make this
>> check, perhaps doing it in incremental sort itself would be better than
>> doing it in explain.
>>
>Thanks anyway for the commit.
>But if you used the first version of my patch, would the author be me and
>am I as reported?
>What does it take to be considered the author?
>

Apologies. I should have listed you as an author, not just in the
reported-by field.

regards

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