Thread: doc fixes: vacuum_cleanup_index_scale_factor

Introduced 857f9c36cda520030381bd8c2af20adf0ce0e1d4

The "minimal version" should probably be "minimum version", but I didn't
include it here.

Also, the documentation doesn't indicate the default value of -1 (or its
special meaning).

Also, my understanding of this feature changed when I read this logic:

    if (cleanup_scale_factor < 0 ||
        metad->btm_last_cleanup_num_heap_tuples < 0 ||
        info->num_heap_tuples > (1.0 + cleanup_scale_factor) *
        metad->btm_last_cleanup_num_heap_tuples)
        result = true;

=> That means that _bt_vacuum_needs_cleanup() returns true unless a nondefault
value is set.  That feels wrong, since the doc said:

    "When no tuples were deleted from the heap, B-tree indexes
    might still be scanned during <command>VACUUM</command>
    cleanup stage by two reasons."

Which sounds like "being scanned when no tuples were deleted" is exceptional
rather, than the default.

I changed that paragraph based on that understanding (which is consistent with
the commit message).  The language could probably be further improved, but
someone should first verify my tentative understanding of the intent.

Justin

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index eabe2a9..18c0ec0 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -1893,14 +1893,15 @@ include_dir 'conf.d'
       </term>
       <listitem>
        <para>
-        When no tuples were deleted from the heap, B-tree indexes might still
-        be scanned during <command>VACUUM</command> cleanup stage by two
-        reasons.  The first reason is that B-tree index contains deleted pages
-        which can be recycled during cleanup.  The second reason is that B-tree
-        index statistics is stalled.  The criterion of stalled index statistics
-        is number of inserted tuples since previous statistics collection
-        is greater than <varname>vacuum_cleanup_index_scale_factor</varname>
-        fraction of total number of heap tuples.
+        When no tuples were deleted from the heap, B-tree indexes are still
+        scanned during <command>VACUUM</command> cleanup stage unless
+        two conditions are met.  First, if a B-tree index contains no deleted pages
+        which can be recycled during cleanup.  Second, if B-tree
+        index statistics are not stale.  Index statistics are considered stale unless
+        <varname>vacuum_cleanup_index_scale_factor</varname> is non-negative, and the
+        number of inserted tuples since the previous statistics collection is
+        less than that fraction of the total number of heap tuples.
+        The default is -1, meaning index scan during cleanup is not skipped.
        </para>
       </listitem>
      </varlistentry>
diff --git a/src/backend/access/nbtree/nbtpage.c b/src/backend/access/nbtree/nbtpage.c
index 3bcc56e..22b4a75 100644
--- a/src/backend/access/nbtree/nbtpage.c
+++ b/src/backend/access/nbtree/nbtpage.c
@@ -189,7 +189,7 @@ _bt_update_meta_cleanup_info(Relation rel, TransactionId oldestBtpoXact,
     if (metad->btm_version < BTREE_VERSION)
         _bt_upgrademetapage(metapg);
 
-    /* update cleanup-related infromation */
+    /* update cleanup-related information */
     metad->btm_oldest_btpo_xact = oldestBtpoXact;
     metad->btm_last_cleanup_num_heap_tuples = numHeapTuples;
     MarkBufferDirty(metabuf);
diff --git a/src/backend/access/nbtree/nbtree.c b/src/backend/access/nbtree/nbtree.c
index e5dce00..4e86280 100644
--- a/src/backend/access/nbtree/nbtree.c
+++ b/src/backend/access/nbtree/nbtree.c
@@ -818,10 +818,10 @@ _bt_vacuum_needs_cleanup(IndexVacuumInfo *info)
         float8        cleanup_scale_factor;
 
         /*
-         * If table receives large enough amount of insertions and no cleanup
-         * was performed, then index might appear to have stalled statistics.
-         * In order to evade that, we perform cleanup when table receives
-         * vacuum_cleanup_index_scale_factor fractions of insertions.
+         * If table receives enough insertions and no cleanup
+         * was performed, then index would appear have stale statistics.
+         * If scale factor is set, we avoid that by performing cleanup if the number of added tuples
+         * exceeds vacuum_cleanup_index_scale_factor fraction of original tuple count.
          */
         relopts = (StdRdOptions *) info->index->rd_options;
         cleanup_scale_factor = (relopts &&
@@ -870,8 +870,8 @@ btbulkdelete(IndexVacuumInfo *info, IndexBulkDeleteResult *stats,
                      &oldestBtpoXact);
 
         /*
-         * Update cleanup-related information in metapage. These information
-         * is used only for cleanup but keeping up them to date can avoid
+         * Update cleanup-related information in metapage. This information
+         * is used only for cleanup but keeping them up to date can avoid
          * unnecessary cleanup even after bulkdelete.
          */
         _bt_update_meta_cleanup_info(info->index, oldestBtpoXact,
@@ -899,8 +899,8 @@ btvacuumcleanup(IndexVacuumInfo *info, IndexBulkDeleteResult *stats)
      * If btbulkdelete was called, we need not do anything, just return the
      * stats from the latest btbulkdelete call.  If it wasn't called, we might
      * still need to do a pass over the index, to recycle any newly-recyclable
-     * pages and to obtain index statistics.  _bt_vacuum_needs_cleanup checks
-     * is there are newly-recyclable or stalled index statistics.
+     * pages or to obtain index statistics.  _bt_vacuum_needs_cleanup
+     * determines if if either are needed.
      *
      * Since we aren't going to actually delete any leaf items, there's no
      * need to go through all the vacuum-cycle-ID pushups.

On Tue, May 1, 2018 at 10:30 PM, Justin Pryzby <pryzby@telsasoft.com> wrote:
> -        When no tuples were deleted from the heap, B-tree indexes might still
> -        be scanned during <command>VACUUM</command> cleanup stage by two
> -        reasons.  The first reason is that B-tree index contains deleted pages
> -        which can be recycled during cleanup.  The second reason is that B-tree
> -        index statistics is stalled.  The criterion of stalled index statistics
> -        is number of inserted tuples since previous statistics collection
> -        is greater than <varname>vacuum_cleanup_index_scale_factor</varname>
> -        fraction of total number of heap tuples.
> +        When no tuples were deleted from the heap, B-tree indexes are still
> +        scanned during <command>VACUUM</command> cleanup stage unless
> +        two conditions are met.  First, if a B-tree index contains no deleted pages
> +        which can be recycled during cleanup.  Second, if B-tree
> +        index statistics are not stale.  Index statistics are considered stale unless
> +        <varname>vacuum_cleanup_index_scale_factor</varname> is non-negative, and the
> +        number of inserted tuples since the previous statistics collection is
> +        less than that fraction of the total number of heap tuples.
> +        The default is -1, meaning index scan during cleanup is not skipped.

I agree that this documentation needs to be rewritten but your rewrite
doesn't strike me as very good English either.  A sentence of the form
"First, if I like hamburgers." is not correct English.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, May 02, 2018 at 10:54:31AM -0400, Robert Haas wrote:
> On Tue, May 1, 2018 at 10:30 PM, Justin Pryzby <pryzby@telsasoft.com> wrote:
> > -        When no tuples were deleted from the heap, B-tree indexes might still
> > -        be scanned during <command>VACUUM</command> cleanup stage by two
> > -        reasons.  The first reason is that B-tree index contains deleted pages
> > -        which can be recycled during cleanup.  The second reason is that B-tree
> > -        index statistics is stalled.  The criterion of stalled index statistics
> > -        is number of inserted tuples since previous statistics collection
> > -        is greater than <varname>vacuum_cleanup_index_scale_factor</varname>
> > -        fraction of total number of heap tuples.
> > +        When no tuples were deleted from the heap, B-tree indexes are still
> > +        scanned during <command>VACUUM</command> cleanup stage unless
> > +        two conditions are met.  First, if a B-tree index contains no deleted pages
> > +        which can be recycled during cleanup.  Second, if B-tree
> > +        index statistics are not stale.  Index statistics are considered stale unless
> > +        <varname>vacuum_cleanup_index_scale_factor</varname> is non-negative, and the
> > +        number of inserted tuples since the previous statistics collection is
> > +        less than that fraction of the total number of heap tuples.
> > +        The default is -1, meaning index scan during cleanup is not skipped.
> 
> I agree that this documentation needs to be rewritten but your rewrite
> doesn't strike me as very good English either.

2nd attempt


index eabe2a9..e305de9 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -1893,14 +1893,16 @@ include_dir 'conf.d'
       </term>
       <listitem>
        <para>
-        When no tuples were deleted from the heap, B-tree indexes might still
-        be scanned during <command>VACUUM</command> cleanup stage by two
-        reasons.  The first reason is that B-tree index contains deleted pages
-        which can be recycled during cleanup.  The second reason is that B-tree
-        index statistics is stalled.  The criterion of stalled index statistics
-        is number of inserted tuples since previous statistics collection
-        is greater than <varname>vacuum_cleanup_index_scale_factor</varname>
-        fraction of total number of heap tuples.
+        When no tuples were deleted from the heap, B-tree indexes are still
+        scanned during <command>VACUUM</command> cleanup stage unless two
+        conditions are met: the index contains no deleted pages which can be
+        recycled during cleanup; and, the index statistics are not stale.
+        Index statistics are considered stale unless
+        <varname>vacuum_cleanup_index_scale_factor</varname>
+        is set to a non-negative value, and the number of inserted tuples since
+        the previous statistics collection is less than that fraction of the
+        total number of heap tuples.  The default is -1, which means index
+        scans during <command>VACUUM</command> cleanup are not skipped.
        </para>
       </listitem>
      </varlistentry>
diff --git a/src/backend/access/nbtree/nbtpage.c b/src/backend/access/nbtree/nbtpage.c
index 3bcc56e..22b4a75 100644
--- a/src/backend/access/nbtree/nbtpage.c
+++ b/src/backend/access/nbtree/nbtpage.c
@@ -189,7 +189,7 @@ _bt_update_meta_cleanup_info(Relation rel, TransactionId oldestBtpoXact,
     if (metad->btm_version < BTREE_VERSION)
         _bt_upgrademetapage(metapg);
 
-    /* update cleanup-related infromation */
+    /* update cleanup-related information */
     metad->btm_oldest_btpo_xact = oldestBtpoXact;
     metad->btm_last_cleanup_num_heap_tuples = numHeapTuples;
     MarkBufferDirty(metabuf);
diff --git a/src/backend/access/nbtree/nbtree.c b/src/backend/access/nbtree/nbtree.c
index e5dce00..4e86280 100644
--- a/src/backend/access/nbtree/nbtree.c
+++ b/src/backend/access/nbtree/nbtree.c
@@ -818,10 +818,10 @@ _bt_vacuum_needs_cleanup(IndexVacuumInfo *info)
         float8        cleanup_scale_factor;
 
         /*
-         * If table receives large enough amount of insertions and no cleanup
-         * was performed, then index might appear to have stalled statistics.
-         * In order to evade that, we perform cleanup when table receives
-         * vacuum_cleanup_index_scale_factor fractions of insertions.
+         * If table receives enough insertions and no cleanup
+         * was performed, then index would appear have stale statistics.
+         * If scale factor is set, we avoid that by performing cleanup if the number of added tuples
+         * exceeds vacuum_cleanup_index_scale_factor fraction of original tuple count.
          */
         relopts = (StdRdOptions *) info->index->rd_options;
         cleanup_scale_factor = (relopts &&
@@ -870,8 +870,8 @@ btbulkdelete(IndexVacuumInfo *info, IndexBulkDeleteResult *stats,
                      &oldestBtpoXact);
 
         /*
-         * Update cleanup-related information in metapage. These information
-         * is used only for cleanup but keeping up them to date can avoid
+         * Update cleanup-related information in metapage. This information
+         * is used only for cleanup but keeping them up to date can avoid
          * unnecessary cleanup even after bulkdelete.
          */
         _bt_update_meta_cleanup_info(info->index, oldestBtpoXact,
@@ -899,8 +899,8 @@ btvacuumcleanup(IndexVacuumInfo *info, IndexBulkDeleteResult *stats)
      * If btbulkdelete was called, we need not do anything, just return the
      * stats from the latest btbulkdelete call.  If it wasn't called, we might
      * still need to do a pass over the index, to recycle any newly-recyclable
-     * pages and to obtain index statistics.  _bt_vacuum_needs_cleanup checks
-     * is there are newly-recyclable or stalled index statistics.
+     * pages or to obtain index statistics.  _bt_vacuum_needs_cleanup
+     * determines if if either are needed.
      *
      * Since we aren't going to actually delete any leaf items, there's no
      * need to go through all the vacuum-cycle-ID pushups.

On Wed, May 2, 2018 at 11:43 AM, Justin Pryzby <pryzby@telsasoft.com> wrote:
> 2nd attempt

That looks like good English to me.  I can't vouch for whether it's
technically correct.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, May 07, 2018 at 07:26:25PM +0300, Alexander Korotkov wrote:
> Hi!
> 
> I've revised docs and comments, and also made some fixes in the code.
> See the attached patchset.
> 
> * 0004-btree-cleanup-docs-comments-fixes.patch
> Documentation and comment improvements from Justin Pryzby
> revised by me.

2nd iteration:

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index eabe2a9235..785ecf922a 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -1893,15 +1893,35 @@ include_dir 'conf.d'
       </term>
       <listitem>
        <para>
-        When no tuples were deleted from the heap, B-tree indexes might still
-        be scanned during <command>VACUUM</command> cleanup stage by two
-        reasons.  The first reason is that B-tree index contains deleted pages
-        which can be recycled during cleanup.  The second reason is that B-tree
-        index statistics is stalled.  The criterion of stalled index statistics
-        is number of inserted tuples since previous statistics collection
-        is greater than <varname>vacuum_cleanup_index_scale_factor</varname>
-        fraction of total number of heap tuples.
+        When no tuples were deleted from the heap, B-tree indexes are still
+        scanned during <command>VACUUM</command> cleanup stage unless two
+        conditions are met: the index contains no deleted pages which can be
+        recycled during cleanup; and, the index statistics are not stale.
+        In order to detect stale index statistics, number of total heap tuples
should say: "THE number"

+        during previous statistics collection is memorized in the index
s/memorized/stored/

+        meta-page.  Once number number of inserted tuples since previous
Should say "Once the number of inserted tuples..."

+        statistics collection is more than
+        <varname>vacuum_cleanup_index_scale_factor</varname> fraction of
+        number of heap tuples memorized in the meta-page, index statistics is
s/memorized/stored/

+        considered to be stalled.  Note, that number of heap tuples is written
"THE number"
s/stalled/stale/

+        to the meta-page at the first time when no dead tuples are found
remove "at"

+        during <command>VACUUM</command> cycle.  Thus, skip of B-tree index
I think should say: "Thus, skipping of the B-tree index scan"

+        scan during cleanup stage is only possible in second and subsequent
s/in/when/

+       <para>
+        Zero value of <varname>vacuum_cleanup_index_scale_factor</varname>
I would say "A zero value of ..."

Thanks,
Justin

3rd iteration ; thanks for bearing with me.

On Tue, May 08, 2018 at 12:35:00PM +0300, Alexander Korotkov wrote:
> Hi, Justin!
> 
> Thank you for revising documentation patch.
> 
> On Mon, May 7, 2018 at 7:55 PM, Justin Pryzby <pryzby@telsasoft.com> wrote:


+        In order to detect stale index statistics, the number of total heap
+        tuples during previous statistics collection is stored in the index
+        meta-page.

Consider removing: "during previous statistics collection"
Or: "during THE previous statistics collection"

+ Once the number of inserted tuples since previous

since THE previous

+        statistics collection is more than
+        <varname>vacuum_cleanup_index_scale_factor</varname> fraction of

Since the multiplier can be greater than 1, should we say "multiple" instead of
fraction?

+        during <command>VACUUM</command> cycle.  Thus, skipping of the B-tree
+        index scan during cleanup stage is only possible when second and
+        subsequent <command>VACUUM</command> cycles detecting no dead tuples.

Change "detecting" to "detect".  Or maybe just "find"

Justin

On 05/08/2018 02:05 PM, Justin Pryzby wrote:
> 3rd iteration ; thanks for bearing with me.
>
> On Tue, May 08, 2018 at 12:35:00PM +0300, Alexander Korotkov wrote:
>> Hi, Justin!
>>
>> Thank you for revising documentation patch.
>>
>> On Mon, May 7, 2018 at 7:55 PM, Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> +        In order to detect stale index statistics, the number of total heap
> +        tuples during previous statistics collection is stored in the index
> +        meta-page.
>
> Consider removing: "during previous statistics collection"
> Or: "during THE previous statistics collection"
>
> + Once the number of inserted tuples since previous
>
> since THE previous
>
> +        statistics collection is more than
> +        <varname>vacuum_cleanup_index_scale_factor</varname> fraction of
>
> Since the multiplier can be greater than 1, should we say "multiple" instead of
> fraction?
>
> +        during <command>VACUUM</command> cycle.  Thus, skipping of the B-tree
> +        index scan during cleanup stage is only possible when second and
> +        subsequent <command>VACUUM</command> cycles detecting no dead tuples.
>
> Change "detecting" to "detect".  Or maybe just "find"
>
> Justin
>
Hi Justin,

Thank you for helping with the docs. Attached is another doc patch that 
should address most of the issues you've brought up.
I've also reshuffled the text a bit to make it more like an option 
description. Hope you'll find it useful.

-- 
Liudmila Mantrova
Technical writer at Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

thanks to everyone, pushed

-- 
Teodor Sigaev                                   E-mail: teodor@sigaev.ru
                                                    WWW: http://www.sigaev.ru/