pgsql: Increase upper limit for vacuum_cleanup_index_scale_factor

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

pgsql: Increase upper limit for vacuum_cleanup_index_scale_factor

Alexander Korotkov-3
Increase upper limit for vacuum_cleanup_index_scale_factor

Upper limits for vacuum_cleanup_index_scale_factor GUC and reloption
were initially set to 100.0 in 857f9c36.  However, after further
discussion, it appears that some users like to disable B-tree cleanup
index scan completely (assuming there are no deleted pages).

vacuum_cleanup_index_scale_factor is used barely to protect against
stalled index statistics.  And after detailed consideration it appears
that risk of stalled index statistics is low.  And it would be nice to
allow advanced users setting higher values of
vacuum_cleanup_index_scale_factor.  So, set upper limit for these
GUC and reloption to DBL_MAX.

Author: Alexander Korotkov
Reviewed-by: Masahiko Sawada
Discussion: https://postgr.es/m/CAC8Q8tJCb%3DgxhzcV7T6ctx7PY-Ux1oA-AsTJc6cAVNsQiYcCzA%40mail.gmail.com

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/6ca33a885bf892a7fa34020a2620c83ccec3cdd7

Modified Files
--------------
doc/src/sgml/config.sgml                  | 2 +-
src/backend/access/common/reloptions.c    | 2 +-
src/backend/access/nbtree/nbtree.c        | 8 +++++---
src/backend/utils/misc/guc.c              | 2 +-
src/test/regress/expected/btree_index.out | 2 +-
5 files changed, 9 insertions(+), 7 deletions(-)

Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Increase upper limit for vacuum_cleanup_index_scale_factor

Alexander Korotkov
On Tue, Jun 26, 2018 at 3:35 PM Alexander Korotkov
<[hidden email]> wrote:
> vacuum_cleanup_index_scale_factor is used barely to protect against
> stalled index statistics.  And after detailed consideration it appears
> that risk of stalled index statistics is low.  And it would be nice to
> allow advanced users setting higher values of
> vacuum_cleanup_index_scale_factor.  So, set upper limit for these
> GUC and reloption to DBL_MAX.

BTW, this line looks cumbersome.

+DETAIL:  Valid values are between "0.000000" and
"179769313486231570814527423731704356798070567525844996598917476803157260780028538760589558632766878171540458953514382464234321326889464182768467546703537516986049910576551282076245490090389328944075868508455133942304583236903222948165808559332123348274797826204144723168738177180919299881250404026184124858368.000000".

It's not something introduced by this patch, because other reloptions
behave the same.  Should we change output format for real reloption
boundaries to '%g' (as guc.c does).  It looks much better.

ERROR:  -1 is outside the valid range for parameter "random_page_cost"
(0 .. 1.79769e+308)

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Increase upper limit for vacuum_cleanup_index_scale_factor

Alexander Korotkov
On Tue, Jun 26, 2018 at 3:59 PM Alexander Korotkov
<[hidden email]> wrote:

>
> On Tue, Jun 26, 2018 at 3:35 PM Alexander Korotkov
> <[hidden email]> wrote:
> > vacuum_cleanup_index_scale_factor is used barely to protect against
> > stalled index statistics.  And after detailed consideration it appears
> > that risk of stalled index statistics is low.  And it would be nice to
> > allow advanced users setting higher values of
> > vacuum_cleanup_index_scale_factor.  So, set upper limit for these
> > GUC and reloption to DBL_MAX.
>
> BTW, this line looks cumbersome.
>
> +DETAIL:  Valid values are between "0.000000" and
> "179769313486231570814527423731704356798070567525844996598917476803157260780028538760589558632766878171540458953514382464234321326889464182768467546703537516986049910576551282076245490090389328944075868508455133942304583236903222948165808559332123348274797826204144723168738177180919299881250404026184124858368.000000".
>
> It's not something introduced by this patch, because other reloptions
> behave the same.  Should we change output format for real reloption
> boundaries to '%g' (as guc.c does).  It looks much better.
>
> ERROR:  -1 is outside the valid range for parameter "random_page_cost"
> (0 .. 1.79769e+308)
See attached patch changing output format for reloption real
boundaries from "%f" to "%g".

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

change-reloption-real-boundaries-output-format.patch (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Increase upper limit for vacuum_cleanup_index_scale_factor

Tom Lane-2
In reply to this post by Alexander Korotkov
Alexander Korotkov <[hidden email]> writes:
> BTW, this line looks cumbersome.

> +DETAIL:  Valid values are between "0.000000" and
> "179769313486231570814527423731704356798070567525844996598917476803157260780028538760589558632766878171540458953514382464234321326889464182768467546703537516986049910576551282076245490090389328944075868508455133942304583236903222948165808559332123348274797826204144723168738177180919299881250404026184124858368.000000".

> It's not something introduced by this patch, because other reloptions
> behave the same.  Should we change output format for real reloption
> boundaries to '%g' (as guc.c does).  It looks much better.
> ERROR:  -1 is outside the valid range for parameter "random_page_cost"
> (0 .. 1.79769e+308)

%g, unmodified, is a bad idea because it loses a lot of precision
in some cases (due to the assumption that nobody cares about more
than six digits).  Maybe you could fix that by using %.15g or some
such, but...

I think that the original patch to use DBL_MAX was itself a bad idea
and should be rethought.  It creates (what is in principle) a
platform-dependent limit, for no adequate justification.  Why not
just set it to 1e9 or 1e10 or some such?

                        regards, tom lane

Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Increase upper limit for vacuum_cleanup_index_scale_factor

Alexander Korotkov
On Tue, Jun 26, 2018 at 7:00 PM Tom Lane <[hidden email]> wrote:

>
> Alexander Korotkov <[hidden email]> writes:
> > BTW, this line looks cumbersome.
>
> > +DETAIL:  Valid values are between "0.000000" and
> > "179769313486231570814527423731704356798070567525844996598917476803157260780028538760589558632766878171540458953514382464234321326889464182768467546703537516986049910576551282076245490090389328944075868508455133942304583236903222948165808559332123348274797826204144723168738177180919299881250404026184124858368.000000".
>
> > It's not something introduced by this patch, because other reloptions
> > behave the same.  Should we change output format for real reloption
> > boundaries to '%g' (as guc.c does).  It looks much better.
> > ERROR:  -1 is outside the valid range for parameter "random_page_cost"
> > (0 .. 1.79769e+308)
>
> %g, unmodified, is a bad idea because it loses a lot of precision
> in some cases (due to the assumption that nobody cares about more
> than six digits).  Maybe you could fix that by using %.15g or some
> such, but...
>
> I think that the original patch to use DBL_MAX was itself a bad idea
> and should be rethought.  It creates (what is in principle) a
> platform-dependent limit, for no adequate justification.  Why not
> just set it to 1e9 or 1e10 or some such?
Yes, I see that it was a bad idea, because many of buildfarm member
are turning red...

I choose DBL_MAX for the sake of uniformity, because we're currently
using DBL_MAX for floating point GUCs and reloptions, which allows
large values.  But we didn't test them for overflow yet...

So, let's switch to 1e10 limit?  Patch is attached.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

vacuum_cleanup_index_scale_factor-max-3.patch (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Increase upper limit for vacuum_cleanup_index_scale_factor

Piotr Stefaniak
In reply to this post by Alexander Korotkov-3
On 26/06/2018 14.35, Alexander Korotkov wrote:

> Increase upper limit for vacuum_cleanup_index_scale_factor
>
> Upper limits for vacuum_cleanup_index_scale_factor GUC and reloption
> were initially set to 100.0 in 857f9c36.  However, after further
> discussion, it appears that some users like to disable B-tree cleanup
> index scan completely (assuming there are no deleted pages).
>
> vacuum_cleanup_index_scale_factor is used barely to protect against
> stalled index statistics.  And after detailed consideration it appears
> that risk of stalled index statistics is low.  And it would be nice to
> allow advanced users setting higher values of
> vacuum_cleanup_index_scale_factor.  So, set upper limit for these
> GUC and reloption to DBL_MAX.
UB Sanitizer points out that prev_num_heap_tuples is sometimes 0,
leading to division by 0 in
                        (info->num_heap_tuples - prev_num_heap_tuples) /
                        prev_num_heap_tuples >= cleanup_scale_factor)
which are currently lines 839-840 in nbtree.c.

Attaching my idea of a fix.

zero-divide-prev_num_heap_tuples.patch (868 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Increase upper limit for vacuum_cleanup_index_scale_factor

Alexander Korotkov
Hi!

On Sun, Apr 14, 2019 at 11:00 PM Piotr Stefaniak
<[hidden email]> wrote:

> On 26/06/2018 14.35, Alexander Korotkov wrote:
> > Increase upper limit for vacuum_cleanup_index_scale_factor
> >
> > Upper limits for vacuum_cleanup_index_scale_factor GUC and reloption
> > were initially set to 100.0 in 857f9c36.  However, after further
> > discussion, it appears that some users like to disable B-tree cleanup
> > index scan completely (assuming there are no deleted pages).
> >
> > vacuum_cleanup_index_scale_factor is used barely to protect against
> > stalled index statistics.  And after detailed consideration it appears
> > that risk of stalled index statistics is low.  And it would be nice to
> > allow advanced users setting higher values of
> > vacuum_cleanup_index_scale_factor.  So, set upper limit for these
> > GUC and reloption to DBL_MAX.
>
> UB Sanitizer points out that prev_num_heap_tuples is sometimes 0,
> leading to division by 0 in
>                         (info->num_heap_tuples - prev_num_heap_tuples) /
>                         prev_num_heap_tuples >= cleanup_scale_factor)
> which are currently lines 839-840 in nbtree.c.
>
> Attaching my idea of a fix.
Thank you for noticing.  BTW, I've more trivial idea for fixing this: replace
prev_num_heap_tuples < 0
with
prev_num_heap_tuples <= 0

If prev_num_heap_tuples == 0, subsequent part of expression isn't
evaluated because result is known to be true.  And I think it's right
to don't skip cleanup when prev_num_heap_tuples == 0.



------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

fix-division-by-zero-bt-vacuum-needs-cleanup.patch (774 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Increase upper limit for vacuum_cleanup_index_scale_factor

Masahiko Sawada
On Mon, Apr 15, 2019 at 11:57 AM Alexander Korotkov
<[hidden email]> wrote:

>
> Hi!
>
> On Sun, Apr 14, 2019 at 11:00 PM Piotr Stefaniak
> <[hidden email]> wrote:
> > On 26/06/2018 14.35, Alexander Korotkov wrote:
> > > Increase upper limit for vacuum_cleanup_index_scale_factor
> > >
> > > Upper limits for vacuum_cleanup_index_scale_factor GUC and reloption
> > > were initially set to 100.0 in 857f9c36.  However, after further
> > > discussion, it appears that some users like to disable B-tree cleanup
> > > index scan completely (assuming there are no deleted pages).
> > >
> > > vacuum_cleanup_index_scale_factor is used barely to protect against
> > > stalled index statistics.  And after detailed consideration it appears
> > > that risk of stalled index statistics is low.  And it would be nice to
> > > allow advanced users setting higher values of
> > > vacuum_cleanup_index_scale_factor.  So, set upper limit for these
> > > GUC and reloption to DBL_MAX.
> >
> > UB Sanitizer points out that prev_num_heap_tuples is sometimes 0,
> > leading to division by 0 in
> >                         (info->num_heap_tuples - prev_num_heap_tuples) /
> >                         prev_num_heap_tuples >= cleanup_scale_factor)
> > which are currently lines 839-840 in nbtree.c.

Thank you pointing out it.

> >
> > Attaching my idea of a fix.
>
> Thank you for noticing.  BTW, I've more trivial idea for fixing this: replace
> prev_num_heap_tuples < 0
> with
> prev_num_heap_tuples <= 0
>
> If prev_num_heap_tuples == 0, subsequent part of expression isn't
> evaluated because result is known to be true.  And I think it's right
> to don't skip cleanup when prev_num_heap_tuples == 0.

+1. It looks good to me.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center


Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Increase upper limit for vacuum_cleanup_index_scale_factor

Alexander Korotkov
On Mon, Apr 15, 2019 at 11:28 AM Masahiko Sawada <[hidden email]> wrote:

> On Mon, Apr 15, 2019 at 11:57 AM Alexander Korotkov
> <[hidden email]> wrote:
> >
> > Hi!
> >
> > On Sun, Apr 14, 2019 at 11:00 PM Piotr Stefaniak
> > <[hidden email]> wrote:
> > > UB Sanitizer points out that prev_num_heap_tuples is sometimes 0,
> > > leading to division by 0 in
> > >                         (info->num_heap_tuples - prev_num_heap_tuples) /
> > >                         prev_num_heap_tuples >= cleanup_scale_factor)
> > > which are currently lines 839-840 in nbtree.c.
>
> Thank you pointing out it.
>
> > >
> > > Attaching my idea of a fix.
> >
> > Thank you for noticing.  BTW, I've more trivial idea for fixing this: replace
> > prev_num_heap_tuples < 0
> > with
> > prev_num_heap_tuples <= 0
> >
> > If prev_num_heap_tuples == 0, subsequent part of expression isn't
> > evaluated because result is known to be true.  And I think it's right
> > to don't skip cleanup when prev_num_heap_tuples == 0.
>
> +1. It looks good to me.

Thank you for the feedback!
Pushed.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company