Thread: pgsql: Increase upper limit for vacuum_cleanup_index_scale_factor
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(-)
On Tue, Jun 26, 2018 at 3:35 PM Alexander Korotkov <akorotkov@postgresql.org> 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
On Tue, Jun 26, 2018 at 3:59 PM Alexander Korotkov <a.korotkov@postgrespro.ru> wrote: > > On Tue, Jun 26, 2018 at 3:35 PM Alexander Korotkov > <akorotkov@postgresql.org> 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
Attachment
Alexander Korotkov <a.korotkov@postgrespro.ru> 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
On Tue, Jun 26, 2018 at 7:00 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Alexander Korotkov <a.korotkov@postgrespro.ru> 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
Attachment
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.
Attachment
Hi! On Sun, Apr 14, 2019 at 11:00 PM Piotr Stefaniak <postgres@piotr-stefaniak.me> 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
Attachment
On Mon, Apr 15, 2019 at 11:57 AM Alexander Korotkov <a.korotkov@postgrespro.ru> wrote: > > Hi! > > On Sun, Apr 14, 2019 at 11:00 PM Piotr Stefaniak > <postgres@piotr-stefaniak.me> 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
On Mon, Apr 15, 2019 at 11:28 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > On Mon, Apr 15, 2019 at 11:57 AM Alexander Korotkov > <a.korotkov@postgrespro.ru> wrote: > > > > Hi! > > > > On Sun, Apr 14, 2019 at 11:00 PM Piotr Stefaniak > > <postgres@piotr-stefaniak.me> 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