Re: [BUGS] BUG #14057: vacuum setting reltuples=0 for tables with >0tuples

On 2017-03-16 06:55:54 +0000, Andrew Gierth wrote:
> >>>>> "Andres" == Andres Freund <andres@anarazel.de> writes:
> 
>  >> It looks like fixing this requires breaking scanned_pages out into
>  >> at least two separate counters, since we currently use it to track
>  >> both whether we can update stats, and whether we can update
>  >> relfrozenxid.
> 
>  Andres> Are you planning to submit a fix?
> 
> Somewhat belatedly, yes.
> 
> Attached are patches against master and all the back branches back to
> 9.2.

Cool.

> I've reproduced the bug on all of them, and confirmed that this
> fixes it on all of them.  Is it worth also including the isolation
> tester script in the changes?

Hm, I haven't seen the isolationtester test (it's not in this thread,
right?) - how fragile and how slow is it?


> diff --git a/src/backend/commands/vacuumlazy.c b/src/backend/commands/vacuumlazy.c
> index 8aefa7aaa9..a33541a115 100644
> --- a/src/backend/commands/vacuumlazy.c
> +++ b/src/backend/commands/vacuumlazy.c
> @@ -100,7 +100,8 @@ typedef struct LVRelStats
>      BlockNumber old_rel_pages;    /* previous value of pg_class.relpages */
>      BlockNumber rel_pages;        /* total number of pages */
>      BlockNumber scanned_pages;    /* number of pages we examined */
> -    double        scanned_tuples; /* counts only tuples on scanned pages */
> +    BlockNumber    tupcount_pages;    /* pages whose tuples we counted */
> +    double        scanned_tuples; /* counts only tuples on tupcount_pages */
>      double        old_rel_tuples; /* previous value of pg_class.reltuples */
>      double        new_rel_tuples; /* new estimated total # of tuples */
>      BlockNumber pages_removed;
> @@ -258,6 +259,10 @@ lazy_vacuum_rel(Relation onerel, VacuumStmt *vacstmt,
>       * density") with nonzero relpages and reltuples=0 (which means "zero
>       * tuple density") unless there's some actual evidence for the latter.
>       *
> +     * It's important that we use tupcount_pages and not scanned_pages for the
> +     * check described above; scanned_pages counts pages where we could not get
> +     * cleanup lock, and which were processed only for frozenxid purposes.
> +     *
>       * We do update relallvisible even in the corner case, since if the table
>       * is all-visible we'd definitely like to know that.  But clamp the value
>       * to be not more than what we're setting relpages to.
> @@ -267,7 +272,7 @@ lazy_vacuum_rel(Relation onerel, VacuumStmt *vacstmt,
>       */
>      new_rel_pages = vacrelstats->rel_pages;
>      new_rel_tuples = vacrelstats->new_rel_tuples;
> -    if (vacrelstats->scanned_pages == 0 && new_rel_pages > 0)
> +    if (vacrelstats->tupcount_pages == 0 && new_rel_pages > 0)
>      {
>          new_rel_pages = vacrelstats->old_rel_pages;
>          new_rel_tuples = vacrelstats->old_rel_tuples;
> @@ -423,6 +428,7 @@ lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats,
>      nblocks = RelationGetNumberOfBlocks(onerel);
>      vacrelstats->rel_pages = nblocks;
>      vacrelstats->scanned_pages = 0;
> +    vacrelstats->tupcount_pages = 0;
>      vacrelstats->nonempty_pages = 0;
>      vacrelstats->latestRemovedXid = InvalidTransactionId;
>  
> @@ -613,6 +619,7 @@ lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats,
>          }
>  
>          vacrelstats->scanned_pages++;
> +        vacrelstats->tupcount_pages++;
>  
>          page = BufferGetPage(buf);
>  
> @@ -999,7 +1006,7 @@ lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats,
>      /* now we can compute the new value for pg_class.reltuples */
>      vacrelstats->new_rel_tuples = vac_estimate_reltuples(onerel, false,
>                                                           nblocks,
> -                                                  vacrelstats->scanned_pages,
> +                                                  vacrelstats->tupcount_pages,
>                                                           num_tuples);
>  
>      /*
> @@ -1264,7 +1271,7 @@ lazy_cleanup_index(Relation indrel,
>  
>      ivinfo.index = indrel;
>      ivinfo.analyze_only = false;
> -    ivinfo.estimated_count = (vacrelstats->scanned_pages < vacrelstats->rel_pages);
> +    ivinfo.estimated_count = (vacrelstats->tupcount_pages < vacrelstats->rel_pages);
>      ivinfo.message_level = elevel;
>      ivinfo.num_heap_tuples = vacrelstats->new_rel_tuples;
>      ivinfo.strategy = vac_strategy;


Without having tested it, this looks sane.

Greetings,

Andres Freund


-- 
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs