Changed CC to pgsql-hackers.
Tom Lane wrote:
> Alvaro Herrera <alvhe...@alvh.no-ip.org> writes:
> > Frankly, I don't like this. I would rather have an instrument->ntuples2
> > rather than these "divide this by nloops, sometimes" schizoid counters.
> > This is already being misused by ON CONFLICT (see "other_path" in
> > show_modifytable_info). But it seems like a correct fix would require
> > more code.
>
> The question then becomes whether to put back nfiltered3, or to do
> something more to your liking. Think I'd vote for the latter.
Doing it properly is not a lot of code actually. Patch attached. ON
CONFLICT is not changed by this patch, but that's a straightforward
change.
Questions:
1. Do we want to back-patch this to 10? I suppose (without checking)
that EXPLAIN ANALYZE is already reporting bogus numbers for parallel
index-only scans, so I think we should do that.
2. Do we want to revert Andrew's test stabilization patch? If I
understand correctly, the problem is the inverse of what was diagnosed:
"any running transaction at the time of the test could prevent pages
from being set as all-visible". That's correct, but the test doesn't
depend on pages being all-visible -- quite the contrary, it depends on
the pages NOT being all-visible (which is why the HeapFetches counts are
all non-zero). Since the pages contain very few tuples, autovacuum
should never process the tables anyway.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 989b6aad67..fe0419311b 100644
*** a/src/backend/commands/explain.c
--- b/src/backend/commands/explain.c
***************
*** 1459,1470 **** ExplainNode(PlanState *planstate, List *ancestors,
show_instrumentation_count("Rows Removed by
Filter", 1,
planstate, es);
if (es->analyze)
! {
! long heapFetches =
! ((IndexOnlyScanState *)
planstate)->ioss_HeapFetches;
!
! ExplainPropertyInteger("Heap Fetches", NULL,
heapFetches, es);
! }
break;
case T_BitmapIndexScan:
show_scan_qual(((BitmapIndexScan *)
plan)->indexqualorig,
--- 1459,1466 ----
show_instrumentation_count("Rows Removed by
Filter", 1,
planstate, es);
if (es->analyze)
! ExplainPropertyInteger("Heap Fetches", NULL,
!
planstate->instrument->ntuples2, es);
break;
case T_BitmapIndexScan:
show_scan_qual(((BitmapIndexScan *)
plan)->indexqualorig,
diff --git a/src/backend/executor/insindex 86252cee1f..fe5d55904d 100644
*** a/src/backend/executor/instrument.c
--- b/src/backend/executor/instrument.c
***************
*** 156,161 **** InstrAggNode(Instrumentation *dst, Instrumentation *add)
--- 156,162 ----
dst->startup += add->startup;
dst->total += add->total;
dst->ntuples += add->ntuples;
+ dst->ntuples2 += add->ntuples2;
dst->nloops += add->nloops;
dst->nfiltered1 += add->nfiltered1;
dst->nfiltered2 += add->nfiltered2;
diff --git a/src/backend/executor/nodeInindex ddc0ae9061..3a02a99621 100644
*** a/src/backend/executor/nodeIndexonlyscan.c
--- b/src/backend/executor/nodeIndexonlyscan.c
***************
*** 162,168 **** IndexOnlyNext(IndexOnlyScanState *node)
/*
* Rats, we have to visit the heap to check visibility.
*/
! node->ioss_HeapFetches++;
tuple = index_fetch_heap(scandesc);
if (tuple == NULL)
continue; /* no visible tuple,
try next index entry */
--- 162,168 ----
/*
* Rats, we have to visit the heap to check visibility.
*/
! InstrCountTuples2(node, 1);
tuple = index_fetch_heap(scandesc);
if (tuple == NULL)
continue; /* no visible tuple,
try next index entry */
***************
*** 509,515 **** ExecInitIndexOnlyScan(IndexOnlyScan *node, EState *estate, int
eflags)
indexstate->ss.ps.plan = (Plan *) node;
indexstate->ss.ps.state = estate;
indexstate->ss.ps.ExecProcNode = ExecIndexOnlyScan;
- indexstate->ioss_HeapFetches = 0;
/*
* Miscellaneous initialization
--- 509,514 ----
diff --git a/src/include/executor/instrument.h index 28eb0093d4..0ba407f83a
100644
*** a/src/include/executor/instrument.h
--- b/src/include/executor/instrument.h
***************
*** 57,62 **** typedef struct Instrumentation
--- 57,63 ----
double startup; /* Total startup time (in
seconds) */
double total; /* Total total time (in
seconds) */
double ntuples; /* Total tuples produced */
+ double ntuples2; /* Secondary generic tuple
counter */
double nloops; /* # of run cycles for this
node */
double nfiltered1; /* # tuples removed by scanqual
or joinqual OR
* # tuples
inserted by MERGE */
diff --git a/src/include/nodes/execnodesindex 06456f07cc..deab875466 100644
*** a/src/include/nodes/execnodes.h
--- b/src/include/nodes/execnodes.h
***************
*** 1004,1009 **** typedef struct PlanState
--- 1004,1014 ----
#define outerPlanState(node) (((PlanState *)(node))->lefttree)
/* Macros for inline access to certain instrumentation counters */
+ #define InstrCountTuples2(node, delta) \
+ do { \
+ if (((PlanState *)(node))->instrument) \
+ ((PlanState *)(node))->instrument->ntuples2 += (delta);
\
+ } while (0)
#define InstrCountFiltered1(node, delta) \
do { \
if (((PlanState *)(node))->instrument) \
***************
*** 1368,1374 **** typedef struct IndexScanState
* RelationDesc index relation descriptor
* ScanDesc index scan descriptor
* VMBuffer buffer in use for visibility map
testing, if any
- * HeapFetches number of tuples we were forced to
fetch from heap
* ioss_PscanLen Size of parallel index-only scan descriptor
* ----------------
*/
--- 1373,1378 ----
