[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

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?

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."

regards,
Ranier Vilela

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

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.

regards,
Ranier Vilela

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

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?

regards,
Ranier Vilela

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

Em sáb., 9 de mai. de 2020 às 17:48, Tomas Vondra <
tomas.vondra@2ndquadrant.com> escreveu:

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.

Apologies accepted.

Thank you.

Ranier VIilela