From: Tom Lane <tgl@sss.pgh.pa.us>
To: David Gould <daveg@sonic.net>
cc: Alexander Kuzmenkov <a.kuzmenkov@postgrespro.ru>,
        pgsql-hackers@lists.postgresql.org,
        Alina Alexeeva <alexeeva@adobe.com>,
        Ullas Lakkur Raghavendra <lakkurra@adobe.com>
Subject: Re: [patch] BUG #15005: ANALYZE can make pg_class.reltuples
 inaccurate.
In-reply-to: <28773.1520357803@sss.pgh.pa.us>
References: <20180117164916.3fdcf2e9@engels>
 <fa8122c2-ea7a-ff38-49ac-e6616aec38ea@postgrespro.ru>
 <20180228192337.67cc3f99@engels>
 <4988c54a-1709-a978-9a43-65f8a1e09e94@postgrespro.ru>
 <18522.1519916974@sss.pgh.pa.us>
 <1386effe-0e6d-fd5a-6f0d-f294e4ef2dda@postgrespro.ru>
 <3240.1519948160@sss.pgh.pa.us> <20180302161840.3530921f@engels>
 <28773.1520357803@sss.pgh.pa.us>
Comments: In-reply-to Tom Lane <tgl@sss.pgh.pa.us>
	message dated "Tue, 06 Mar 2018 12:36:43 -0500"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <6920.1520871615.0@sss.pgh.pa.us>
Date: Mon, 12 Mar 2018 12:21:34 -0400
Message-ID: <6981.1520871694@sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <6920.1520871615.1@sss.pgh.pa.us>

I wrote:
> Maybe this type of situation is an argument for trusting an ANALYZE-based
> estimate more than the VACUUM-based estimate.  I remain uncomfortable with
> that in cases where VACUUM looked at much more of the table than ANALYZE
> did, though.  Maybe we need some heuristic based on the number of pages
> actually visited by each pass?

I looked into doing something like that.  It's possible, but it's fairly
invasive; there's no clean way to compare those page counts without
altering the API of acquire_sample_rows() to pass back the number of pages
it visited.  That would mean a change in FDW-visible APIs.  We could do
that, but I don't want to insist on it when there's nothing backing it up
except a fear that *maybe* ANALYZE's estimate will be wrong often enough
to worry about.

So at this point I'm prepared to go forward with your patch, though not
to risk back-patching it.  Field experience will tell us if we need to
do more.  I propose the attached cosmetic refactoring, though.

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="analyze-reltuples-change-2.patch";
	charset="us-ascii"
Content-ID: <6920.1520871615.2@sss.pgh.pa.us>
Content-Description: analyze-reltuples-change-2.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/contrib/pgstattuple/pgstatapprox.c b/contrib/pgstattuple/pgst=
atapprox.c
index 3cfbc08..474c3bd 100644
*** a/contrib/pgstattuple/pgstatapprox.c
--- b/contrib/pgstattuple/pgstatapprox.c
*************** statapprox_heap(Relation rel, output_typ
*** 184,190 ****
  =

  	stat->table_len =3D (uint64) nblocks * BLCKSZ;
  =

! 	stat->tuple_count =3D vac_estimate_reltuples(rel, false, nblocks, scann=
ed,
  											   stat->tuple_count + misc_count);
  =

  	/*
--- 184,190 ----
  =

  	stat->table_len =3D (uint64) nblocks * BLCKSZ;
  =

! 	stat->tuple_count =3D vac_estimate_reltuples(rel, nblocks, scanned,
  											   stat->tuple_count + misc_count);
  =

  	/*
diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze=
.c
index 5f21fcb..ef93fb4 100644
*** a/src/backend/commands/analyze.c
--- b/src/backend/commands/analyze.c
*************** acquire_sample_rows(Relation onerel, int
*** 1249,1267 ****
  		qsort((void *) rows, numrows, sizeof(HeapTuple), compare_rows);
  =

  	/*
! 	 * Estimate total numbers of rows in relation.  For live rows, use
! 	 * vac_estimate_reltuples; for dead rows, we have no source of old
! 	 * information, so we have to assume the density is the same in unseen
! 	 * pages as in the pages we scanned.
  	 */
- 	*totalrows =3D vac_estimate_reltuples(onerel, true,
- 										totalblocks,
- 										bs.m,
- 										liverows);
  	if (bs.m > 0)
  		*totaldeadrows =3D floor((deadrows / bs.m) * totalblocks + 0.5);
  	else
  		*totaldeadrows =3D 0.0;
  =

  	/*
  	 * Emit some interesting relation info
--- 1249,1270 ----
  		qsort((void *) rows, numrows, sizeof(HeapTuple), compare_rows);
  =

  	/*
! 	 * Estimate total numbers of live and dead rows in relation, extrapolat=
ing
! 	 * on the assumption that the average tuple density in pages we didn't
! 	 * scan is the same as in the pages we did scan.  Since what we scanned=
 is
! 	 * a random sample of the pages in the relation, this should be a good
! 	 * assumption.
  	 */
  	if (bs.m > 0)
+ 	{
+ 		*totalrows =3D floor((liverows / bs.m) * totalblocks + 0.5);
  		*totaldeadrows =3D floor((deadrows / bs.m) * totalblocks + 0.5);
+ 	}
  	else
+ 	{
+ 		*totalrows =3D 0.0;
  		*totaldeadrows =3D 0.0;
+ 	}
  =

  	/*
  	 * Emit some interesting relation info
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 7aca69a..b50c554 100644
*** a/src/backend/commands/vacuum.c
--- b/src/backend/commands/vacuum.c
*************** vacuum_set_xid_limits(Relation rel,
*** 766,781 ****
   * vac_estimate_reltuples() -- estimate the new value for pg_class.reltu=
ples
   *
   *		If we scanned the whole relation then we should just use the count o=
f
!  *		live tuples seen; but if we did not, we should not trust the count
!  *		unreservedly, especially not in VACUUM, which may have scanned a qui=
te
!  *		nonrandom subset of the table.  When we have only partial informatio=
n,
!  *		we take the old value of pg_class.reltuples as a measurement of the
   *		tuple density in the unscanned pages.
-  *
-  *		This routine is shared by VACUUM and ANALYZE.
   */
  double
! vac_estimate_reltuples(Relation relation, bool is_analyze,
  					   BlockNumber total_pages,
  					   BlockNumber scanned_pages,
  					   double scanned_tuples)
--- 766,779 ----
   * vac_estimate_reltuples() -- estimate the new value for pg_class.reltu=
ples
   *
   *		If we scanned the whole relation then we should just use the count o=
f
!  *		live tuples seen; but if we did not, we should not blindly extrapola=
te
!  *		from that number, since VACUUM may have scanned a quite nonrandom
!  *		subset of the table.  When we have only partial information, we take
!  *		the old value of pg_class.reltuples as a measurement of the
   *		tuple density in the unscanned pages.
   */
  double
! vac_estimate_reltuples(Relation relation,
  					   BlockNumber total_pages,
  					   BlockNumber scanned_pages,
  					   double scanned_tuples)
*************** vac_estimate_reltuples(Relation relation
*** 783,791 ****
  	BlockNumber old_rel_pages =3D relation->rd_rel->relpages;
  	double		old_rel_tuples =3D relation->rd_rel->reltuples;
  	double		old_density;
! 	double		new_density;
! 	double		multiplier;
! 	double		updated_density;
  =

  	/* If we did scan the whole table, just use the count as-is */
  	if (scanned_pages >=3D total_pages)
--- 781,788 ----
  	BlockNumber old_rel_pages =3D relation->rd_rel->relpages;
  	double		old_rel_tuples =3D relation->rd_rel->reltuples;
  	double		old_density;
! 	double		unscanned_pages;
! 	double		total_tuples;
  =

  	/* If we did scan the whole table, just use the count as-is */
  	if (scanned_pages >=3D total_pages)
*************** vac_estimate_reltuples(Relation relation
*** 809,839 ****
  =

  	/*
  	 * Okay, we've covered the corner cases.  The normal calculation is to
! 	 * convert the old measurement to a density (tuples per page), then upd=
ate
! 	 * the density using an exponential-moving-average approach, and finall=
y
! 	 * compute reltuples as updated_density * total_pages.
! 	 *
! 	 * For ANALYZE, the moving average multiplier is just the fraction of t=
he
! 	 * table's pages we scanned.  This is equivalent to assuming that the
! 	 * tuple density in the unscanned pages didn't change.  Of course, it
! 	 * probably did, if the new density measurement is different. But over
! 	 * repeated cycles, the value of reltuples will converge towards the
! 	 * correct value, if repeated measurements show the same new density.
! 	 *
! 	 * For VACUUM, the situation is a bit different: we have looked at a
! 	 * nonrandom sample of pages, but we know for certain that the pages we
! 	 * didn't look at are precisely the ones that haven't changed lately.
! 	 * Thus, there is a reasonable argument for doing exactly the same thin=
g
! 	 * as for the ANALYZE case, that is use the old density measurement as =
the
! 	 * value for the unscanned pages.
! 	 *
! 	 * This logic could probably use further refinement.
  	 */
  	old_density =3D old_rel_tuples / old_rel_pages;
! 	new_density =3D scanned_tuples / scanned_pages;
! 	multiplier =3D (double) scanned_pages / (double) total_pages;
! 	updated_density =3D old_density + (new_density - old_density) * multipl=
ier;
! 	return floor(updated_density * total_pages + 0.5);
  }
  =

  =

--- 806,819 ----
  =

  	/*
  	 * Okay, we've covered the corner cases.  The normal calculation is to
! 	 * convert the old measurement to a density (tuples per page), then
! 	 * estimate the number of tuples in the unscanned pages using that figu=
re,
! 	 * and finally add on the number of tuples in the scanned pages.
  	 */
  	old_density =3D old_rel_tuples / old_rel_pages;
! 	unscanned_pages =3D (double) total_pages - (double) scanned_pages;
! 	total_tuples =3D old_density * unscanned_pages + scanned_tuples;
! 	return floor(total_tuples + 0.5);
  }
  =

  =

diff --git a/src/backend/commands/vacuumlazy.c b/src/backend/commands/vacu=
umlazy.c
index cf7f5e1..9ac84e8 100644
*** a/src/backend/commands/vacuumlazy.c
--- b/src/backend/commands/vacuumlazy.c
*************** lazy_scan_heap(Relation onerel, int opti
*** 1286,1292 ****
  	vacrelstats->new_dead_tuples =3D nkeep;
  =

  	/* now we can compute the new value for pg_class.reltuples */
! 	vacrelstats->new_rel_tuples =3D vac_estimate_reltuples(onerel, false,
  														 nblocks,
  														 vacrelstats->tupcount_pages,
  														 num_tuples);
--- 1286,1292 ----
  	vacrelstats->new_dead_tuples =3D nkeep;
  =

  	/* now we can compute the new value for pg_class.reltuples */
! 	vacrelstats->new_rel_tuples =3D vac_estimate_reltuples(onerel,
  														 nblocks,
  														 vacrelstats->tupcount_pages,
  														 num_tuples);
diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h
index 797b6df..85d472f 100644
*** a/src/include/commands/vacuum.h
--- b/src/include/commands/vacuum.h
*************** extern void vacuum(int options, List *re
*** 162,168 ****
  extern void vac_open_indexes(Relation relation, LOCKMODE lockmode,
  				 int *nindexes, Relation **Irel);
  extern void vac_close_indexes(int nindexes, Relation *Irel, LOCKMODE loc=
kmode);
! extern double vac_estimate_reltuples(Relation relation, bool is_analyze,
  					   BlockNumber total_pages,
  					   BlockNumber scanned_pages,
  					   double scanned_tuples);
--- 162,168 ----
  extern void vac_open_indexes(Relation relation, LOCKMODE lockmode,
  				 int *nindexes, Relation **Irel);
  extern void vac_close_indexes(int nindexes, Relation *Irel, LOCKMODE loc=
kmode);
! extern double vac_estimate_reltuples(Relation relation,
  					   BlockNumber total_pages,
  					   BlockNumber scanned_pages,
  					   double scanned_tuples);

------- =_aaaaaaaaaa0--