Hi,
Le mer. 4 sept. 2024 à 10:47, Bertrand Drouvot <bertranddrouvot...@gmail.com>
a écrit :
> Hi,
>
> On Thu, Aug 29, 2024 at 04:04:05PM +0200, Guillaume Lelarge wrote:
> > Hello,
> >
> > This patch was a bit discussed on [1], and with more details on [2]. It
> > introduces four new columns in pg_stat_all_tables:
> >
> > * parallel_seq_scan
> > * last_parallel_seq_scan
> > * parallel_idx_scan
> > * last_parallel_idx_scan
> >
> > and two new columns in pg_stat_all_indexes:
> >
> > * parallel_idx_scan
> > * last_parallel_idx_scan
> >
> > As Benoit said yesterday, the intent is to help administrators evaluate
> the
> > usage of parallel workers in their databases and help configuring
> > parallelization usage.
>
> Thanks for the patch. I think that's a good idea to provide more
> instrumentation
> in this area. So, +1 regarding this patch.
>
>
Thanks.
> A few random comments:
>
> 1 ===
>
> + <row>
> + <entry role="catalog_table_entry"><para role="column_definition">
> + <structfield>parallel_seq_scan</structfield> <type>bigint</type>
> + </para>
> + <para>
> + Number of parallel sequential scans initiated on this table
> + </para></entry>
> + </row>
>
> I wonder if we should not update the seq_scan too to indicate that it
> includes the parallel_seq_scan.
>
> Same kind of comment for last_seq_scan, idx_scan and last_idx_scan.
>
>
Yeah, not sure why I didn't do it at first. I was wondering the same thing.
The patch attached does this.
> 2 ===
>
> @@ -410,6 +410,8 @@ initscan(HeapScanDesc scan, ScanKey key, bool
> keep_startblock)
> */
> if (scan->rs_base.rs_flags & SO_TYPE_SEQSCAN)
> pgstat_count_heap_scan(scan->rs_base.rs_rd);
> + if (scan->rs_base.rs_parallel != NULL)
> + pgstat_count_parallel_heap_scan(scan->rs_base.rs_rd);
>
> Indentation seems broken.
>
>
My bad, sorry. Fixed in the attached patch.
> Shouldn't the parallel counter relies on the "scan->rs_base.rs_flags &
> SO_TYPE_SEQSCAN"
> test too?
>
>
You're right. Fixed in the attached patch.
> What about to get rid of the pgstat_count_parallel_heap_scan and add an
> extra
> bolean parameter to pgstat_count_heap_scan to indicate if
> counts.parallelnumscans
> should be incremented too?
>
> Something like:
>
> pgstat_count_heap_scan(scan->rs_base.rs_rd, scan->rs_base.rs_parallel !=
> NULL)
>
> 3 ===
>
> Same comment for pgstat_count_index_scan (add an extra bolean parameter)
> and
> get rid of pgstat_count_parallel_index_scan()).
>
> I think that 2 === and 3 === would help to avoid missing increments should
> we
> add those call to other places in the future.
>
>
Oh OK, understood. Done for both.
4 ===
>
> + if (lstats->counts.numscans || lstats->counts.parallelnumscans)
>
> Is it possible to have (lstats->counts.parallelnumscans) whithout having
> (lstats->counts.numscans) ?
>
>
Nope, parallel scans are included in seq/index scans, as far as I can tell.
I could remove the parallelnumscans testing but it would be less obvious to
read.
> > First time I had to add new columns to a statistics catalog. I'm actually
> > not sure that we were right to change pg_proc.dat manually.
>
> I think that's the right way to do.
>
>
OK, new patch attached.
> I don't see a CF entry for this patch. Would you mind creating one so that
> we don't lost track of it?
>
>
I don't mind adding it, though I don't know if I should add it to the
September or November commit fest. Which one should I choose?
Thanks.
Regards.
--
Guillaume.
From 6a202b7bd44cf33be13a8f7e0a8dc7077604c3c0 Mon Sep 17 00:00:00 2001
From: Guillaume Lelarge <guillaume.lela...@dalibo.com>
Date: Wed, 28 Aug 2024 21:35:30 +0200
Subject: [PATCH v2] Add parallel columns for pg_stat_all_tables,indexes
pg_stat_all_tables gets 4 new columns: parallel_seq_scan,
last_parallel_seq_scan, parallel_idx_scan, last_parallel_idx_scan.
pg_stat_all_indexes gets 2 new columns: parallel_idx_scan,
last_parallel_idx_scan.
---
doc/src/sgml/monitoring.sgml | 69 ++++++++++++++++++--
src/backend/access/brin/brin.c | 2 +-
src/backend/access/gin/ginscan.c | 2 +-
src/backend/access/gist/gistget.c | 4 +-
src/backend/access/hash/hashsearch.c | 2 +-
src/backend/access/heap/heapam.c | 2 +-
src/backend/access/nbtree/nbtsearch.c | 2 +-
src/backend/access/spgist/spgscan.c | 2 +-
src/backend/catalog/system_views.sql | 6 ++
src/backend/utils/activity/pgstat_relation.c | 7 +-
src/backend/utils/adt/pgstatfuncs.c | 6 ++
src/include/catalog/pg_proc.dat | 8 +++
src/include/pgstat.h | 23 +++++--
13 files changed, 113 insertions(+), 22 deletions(-)
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 933de6fe07..6886094095 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -3773,7 +3773,7 @@ description | Waiting for a newly initialized WAL file to reach durable storage
<structfield>seq_scan</structfield> <type>bigint</type>
</para>
<para>
- Number of sequential scans initiated on this table
+ Number of sequential scans (including parallel ones) initiated on this table
</para></entry>
</row>
@@ -3782,7 +3782,26 @@ description | Waiting for a newly initialized WAL file to reach durable storage
<structfield>last_seq_scan</structfield> <type>timestamp with time zone</type>
</para>
<para>
- The time of the last sequential scan on this table, based on the
+ The time of the last sequential scan (including parallel ones) on this table, based on the
+ most recent transaction stop time
+ </para></entry>
+ </row>
+
+ <row>
+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>parallel_seq_scan</structfield> <type>bigint</type>
+ </para>
+ <para>
+ Number of parallel sequential scans initiated on this table
+ </para></entry>
+ </row>
+
+ <row>
+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>last_parallel_seq_scan</structfield> <type>timestamp with time zone</type>
+ </para>
+ <para>
+ The time of the last parallel sequential scan on this table, based on the
most recent transaction stop time
</para></entry>
</row>
@@ -3801,7 +3820,7 @@ description | Waiting for a newly initialized WAL file to reach durable storage
<structfield>idx_scan</structfield> <type>bigint</type>
</para>
<para>
- Number of index scans initiated on this table
+ Number of index scans (including parallel ones) initiated on this table
</para></entry>
</row>
@@ -3810,7 +3829,26 @@ description | Waiting for a newly initialized WAL file to reach durable storage
<structfield>last_idx_scan</structfield> <type>timestamp with time zone</type>
</para>
<para>
- The time of the last index scan on this table, based on the
+ The time of the last index scan (including parallel ones) on this table, based on the
+ most recent transaction stop time
+ </para></entry>
+ </row>
+
+ <row>
+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>parallel_idx_scan</structfield> <type>bigint</type>
+ </para>
+ <para>
+ Number of parallel index scans initiated on this table
+ </para></entry>
+ </row>
+
+ <row>
+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>last_parallel_idx_scan</structfield> <type>timestamp with time zone</type>
+ </para>
+ <para>
+ The time of the last parallel index scan on this table, based on the
most recent transaction stop time
</para></entry>
</row>
@@ -4080,7 +4118,7 @@ description | Waiting for a newly initialized WAL file to reach durable storage
<structfield>idx_scan</structfield> <type>bigint</type>
</para>
<para>
- Number of index scans initiated on this index
+ Number of index scans (including parallel ones) initiated on this index
</para></entry>
</row>
@@ -4089,7 +4127,26 @@ description | Waiting for a newly initialized WAL file to reach durable storage
<structfield>last_idx_scan</structfield> <type>timestamp with time zone</type>
</para>
<para>
- The time of the last scan on this index, based on the
+ The time of the last scan on this index(including parallel ones), based
+ on the most recent transaction stop time
+ </para></entry>
+ </row>
+
+ <row>
+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>parallel_idx_scan</structfield> <type>bigint</type>
+ </para>
+ <para>
+ Number of parallel index scans initiated on this index
+ </para></entry>
+ </row>
+
+ <row>
+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>last_parallel_idx_scan</structfield> <type>timestamp with time zone</type>
+ </para>
+ <para>
+ The time of the last parallel scan on this index, based on the
most recent transaction stop time
</para></entry>
</row>
diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index 6467bed604..4b5557fcf7 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -580,7 +580,7 @@ bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm)
opaque = (BrinOpaque *) scan->opaque;
bdesc = opaque->bo_bdesc;
- pgstat_count_index_scan(idxRel);
+ pgstat_count_index_scan(idxRel, false);
/*
* We need to know the size of the table so that we know how long to
diff --git a/src/backend/access/gin/ginscan.c b/src/backend/access/gin/ginscan.c
index af24d38544..2926a4caf6 100644
--- a/src/backend/access/gin/ginscan.c
+++ b/src/backend/access/gin/ginscan.c
@@ -435,7 +435,7 @@ ginNewScanKey(IndexScanDesc scan)
MemoryContextSwitchTo(oldCtx);
- pgstat_count_index_scan(scan->indexRelation);
+ pgstat_count_index_scan(scan->indexRelation, false);
}
void
diff --git a/src/backend/access/gist/gistget.c b/src/backend/access/gist/gistget.c
index b35b8a9757..7e89382ce5 100644
--- a/src/backend/access/gist/gistget.c
+++ b/src/backend/access/gist/gistget.c
@@ -624,7 +624,7 @@ gistgettuple(IndexScanDesc scan, ScanDirection dir)
/* Begin the scan by processing the root page */
GISTSearchItem fakeItem;
- pgstat_count_index_scan(scan->indexRelation);
+ pgstat_count_index_scan(scan->indexRelation, false);
so->firstCall = false;
so->curPageData = so->nPageData = 0;
@@ -749,7 +749,7 @@ gistgetbitmap(IndexScanDesc scan, TIDBitmap *tbm)
if (!so->qual_ok)
return 0;
- pgstat_count_index_scan(scan->indexRelation);
+ pgstat_count_index_scan(scan->indexRelation, false);
/* Begin the scan by processing the root page */
so->curPageData = so->nPageData = 0;
diff --git a/src/backend/access/hash/hashsearch.c b/src/backend/access/hash/hashsearch.c
index 0d99d6abc8..a63edc8372 100644
--- a/src/backend/access/hash/hashsearch.c
+++ b/src/backend/access/hash/hashsearch.c
@@ -297,7 +297,7 @@ _hash_first(IndexScanDesc scan, ScanDirection dir)
HashPageOpaque opaque;
HashScanPosItem *currItem;
- pgstat_count_index_scan(rel);
+ pgstat_count_index_scan(rel, false);
/*
* We do not support hash scans with no index qualification, because we
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 91b20147a0..f21cf50e6e 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -409,7 +409,7 @@ initscan(HeapScanDesc scan, ScanKey key, bool keep_startblock)
* and for sample scans we update stats for tuple fetches).
*/
if (scan->rs_base.rs_flags & SO_TYPE_SEQSCAN)
- pgstat_count_heap_scan(scan->rs_base.rs_rd);
+ pgstat_count_heap_scan(scan->rs_base.rs_rd, (scan->rs_base.rs_parallel != NULL));
}
/*
diff --git a/src/backend/access/nbtree/nbtsearch.c b/src/backend/access/nbtree/nbtsearch.c
index 2551df8a67..ef50852199 100644
--- a/src/backend/access/nbtree/nbtsearch.c
+++ b/src/backend/access/nbtree/nbtsearch.c
@@ -896,7 +896,7 @@ _bt_first(IndexScanDesc scan, ScanDirection dir)
Assert(!BTScanPosIsValid(so->currPos));
- pgstat_count_index_scan(rel);
+ pgstat_count_index_scan(rel, (scan->parallel_scan != NULL));
/*
* Examine the scan keys and eliminate any redundant keys; also mark the
diff --git a/src/backend/access/spgist/spgscan.c b/src/backend/access/spgist/spgscan.c
index 03293a7816..a78fa34570 100644
--- a/src/backend/access/spgist/spgscan.c
+++ b/src/backend/access/spgist/spgscan.c
@@ -422,7 +422,7 @@ spgrescan(IndexScanDesc scan, ScanKey scankey, int nscankeys,
resetSpGistScanOpaque(so);
/* count an indexscan for stats */
- pgstat_count_index_scan(scan->indexRelation);
+ pgstat_count_index_scan(scan->indexRelation, false);
}
void
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 7fd5d256a1..54b1cd6b40 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -670,9 +670,13 @@ CREATE VIEW pg_stat_all_tables AS
C.relname AS relname,
pg_stat_get_numscans(C.oid) AS seq_scan,
pg_stat_get_lastscan(C.oid) AS last_seq_scan,
+ pg_stat_get_parallelnumscans(C.oid) AS parallel_seq_scan,
+ pg_stat_get_parallellastscan(C.oid) AS last_parallel_seq_scan,
pg_stat_get_tuples_returned(C.oid) AS seq_tup_read,
sum(pg_stat_get_numscans(I.indexrelid))::bigint AS idx_scan,
max(pg_stat_get_lastscan(I.indexrelid)) AS last_idx_scan,
+ sum(pg_stat_get_parallelnumscans(I.indexrelid))::bigint AS parallel_idx_scan,
+ max(pg_stat_get_parallellastscan(I.indexrelid)) AS last_parallel_idx_scan,
sum(pg_stat_get_tuples_fetched(I.indexrelid))::bigint +
pg_stat_get_tuples_fetched(C.oid) AS idx_tup_fetch,
pg_stat_get_tuples_inserted(C.oid) AS n_tup_ins,
@@ -792,6 +796,8 @@ CREATE VIEW pg_stat_all_indexes AS
I.relname AS indexrelname,
pg_stat_get_numscans(I.oid) AS idx_scan,
pg_stat_get_lastscan(I.oid) AS last_idx_scan,
+ pg_stat_get_parallelnumscans(I.oid) AS parallel_idx_scan,
+ pg_stat_get_parallellastscan(I.oid) AS last_parallel_idx_scan,
pg_stat_get_tuples_returned(I.oid) AS idx_tup_read,
pg_stat_get_tuples_fetched(I.oid) AS idx_tup_fetch
FROM pg_class C JOIN
diff --git a/src/backend/utils/activity/pgstat_relation.c b/src/backend/utils/activity/pgstat_relation.c
index 8a3f7d434c..cfdc1d42bf 100644
--- a/src/backend/utils/activity/pgstat_relation.c
+++ b/src/backend/utils/activity/pgstat_relation.c
@@ -829,12 +829,15 @@ pgstat_relation_flush_cb(PgStat_EntryRef *entry_ref, bool nowait)
tabentry = &shtabstats->stats;
tabentry->numscans += lstats->counts.numscans;
- if (lstats->counts.numscans)
+ tabentry->parallelnumscans += lstats->counts.parallelnumscans;
+ if (lstats->counts.numscans || lstats->counts.parallelnumscans)
{
TimestampTz t = GetCurrentTransactionStopTimestamp();
- if (t > tabentry->lastscan)
+ if (t > tabentry->lastscan && lstats->counts.numscans)
tabentry->lastscan = t;
+ if (t > tabentry->parallellastscan && lstats->counts.parallelnumscans)
+ tabentry->parallellastscan = t;
}
tabentry->tuples_returned += lstats->counts.tuples_returned;
tabentry->tuples_fetched += lstats->counts.tuples_fetched;
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index 97dc09ac0d..30a3849e3d 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -82,6 +82,9 @@ PG_STAT_GET_RELENTRY_INT64(mod_since_analyze)
/* pg_stat_get_numscans */
PG_STAT_GET_RELENTRY_INT64(numscans)
+/* pg_stat_get_parallelnumscans */
+PG_STAT_GET_RELENTRY_INT64(parallelnumscans)
+
/* pg_stat_get_tuples_deleted */
PG_STAT_GET_RELENTRY_INT64(tuples_deleted)
@@ -140,6 +143,9 @@ PG_STAT_GET_RELENTRY_TIMESTAMPTZ(last_vacuum_time)
/* pg_stat_get_lastscan */
PG_STAT_GET_RELENTRY_TIMESTAMPTZ(lastscan)
+/* pg_stat_get_parallellastscan */
+PG_STAT_GET_RELENTRY_TIMESTAMPTZ(parallellastscan)
+
Datum
pg_stat_get_function_calls(PG_FUNCTION_ARGS)
{
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index ff5436acac..1cce03a6d2 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -5391,6 +5391,14 @@
proname => 'pg_stat_get_lastscan', provolatile => 's', proparallel => 'r',
prorettype => 'timestamptz', proargtypes => 'oid',
prosrc => 'pg_stat_get_lastscan' },
+{ oid => '9000', descr => 'statistics: number of parallel scans done for table/index',
+ proname => 'pg_stat_get_parallelnumscans', provolatile => 's', proparallel => 'r',
+ prorettype => 'int8', proargtypes => 'oid',
+ prosrc => 'pg_stat_get_parallelnumscans' },
+{ oid => '9001', descr => 'statistics: time of the last parallel scan for table/index',
+ proname => 'pg_stat_get_parallellastscan', provolatile => 's', proparallel => 'r',
+ prorettype => 'timestamptz', proargtypes => 'oid',
+ prosrc => 'pg_stat_get_parallellastscan' },
{ oid => '1929', descr => 'statistics: number of tuples read by seqscan',
proname => 'pg_stat_get_tuples_returned', provolatile => 's',
proparallel => 'r', prorettype => 'int8', proargtypes => 'oid',
diff --git a/src/include/pgstat.h b/src/include/pgstat.h
index be2c91168a..c7f4cc3c57 100644
--- a/src/include/pgstat.h
+++ b/src/include/pgstat.h
@@ -192,6 +192,7 @@ typedef struct PgStat_BackendSubEntry
typedef struct PgStat_TableCounts
{
PgStat_Counter numscans;
+ PgStat_Counter parallelnumscans;
PgStat_Counter tuples_returned;
PgStat_Counter tuples_fetched;
@@ -433,6 +434,8 @@ typedef struct PgStat_StatTabEntry
{
PgStat_Counter numscans;
TimestampTz lastscan;
+ PgStat_Counter parallelnumscans;
+ TimestampTz parallellastscan;
PgStat_Counter tuples_returned;
PgStat_Counter tuples_fetched;
@@ -640,10 +643,14 @@ extern void pgstat_report_analyze(Relation rel,
/* nontransactional event counts are simple enough to inline */
-#define pgstat_count_heap_scan(rel) \
+#define pgstat_count_heap_scan(rel, parallel) \
do { \
- if (pgstat_should_count_relation(rel)) \
- (rel)->pgstat_info->counts.numscans++; \
+ if (pgstat_should_count_relation(rel)) { \
+ if (!parallel) \
+ (rel)->pgstat_info->counts.numscans++; \
+ else \
+ (rel)->pgstat_info->counts.parallelnumscans++; \
+ } \
} while (0)
#define pgstat_count_heap_getnext(rel) \
do { \
@@ -655,10 +662,14 @@ extern void pgstat_report_analyze(Relation rel,
if (pgstat_should_count_relation(rel)) \
(rel)->pgstat_info->counts.tuples_fetched++; \
} while (0)
-#define pgstat_count_index_scan(rel) \
+#define pgstat_count_index_scan(rel, parallel) \
do { \
- if (pgstat_should_count_relation(rel)) \
- (rel)->pgstat_info->counts.numscans++; \
+ if (pgstat_should_count_relation(rel)) { \
+ if (!parallel) \
+ (rel)->pgstat_info->counts.numscans++; \
+ else \
+ (rel)->pgstat_info->counts.parallelnumscans++; \
+ } \
} while (0)
#define pgstat_count_index_tuples(rel, n) \
do { \
--
2.46.0
