On 2021-02-04 11:19, Kyotaro Horiguchi wrote:
At Thu, 04 Feb 2021 10:16:47 +0900, torikoshia
<torikos...@oss.nttdata.com> wrote in
Chengxi Sun, Yamada-san, Horiguchi-san,
Thanks for all your comments.
Adding only the number of generic plan execution seems acceptable.
On Mon, Jan 25, 2021 at 2:10 PM Kyotaro Horiguchi
<horikyota....@gmail.com> wrote:
> Note that ActivePortal is the closest nested portal. So it gives the
> wrong result for nested portals.
I may be wrong, but I thought it was ok since the closest nested
portal is the portal to be executed.
After executing the inner-most portal, is_plan_type_generic has a
value for the inner-most portal and it won't be changed ever after. At
the ExecutorEnd of all the upper-portals see the value for the
inner-most portal left behind is_plan_type_generic nevertheless the
portals at every nest level are independent.
ActivePortal is used in ExecutorStart hook in the patch.
And as far as I read PortalStart(), ActivePortal is changed to the
portal to be executed before ExecutorStart().
If possible, could you tell me the specific case which causes wrong
results?
Running a plpgsql function that does PREPRE in a query that does
PREPARE?
Thanks for your explanation!
I confirmed that it in fact happened.
To avoid it, attached patch preserves the is_plan_type_generic before
changing it and sets it back at the end of pgss_ExecutorEnd().
Any thoughts?
Regards,
--
Atsushi Torikoshi
diff --git a/contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql b/contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql
index 0f63f08f7e..7fdef315ae 100644
--- a/contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql
+++ b/contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql
@@ -44,7 +44,8 @@ CREATE FUNCTION pg_stat_statements(IN showtext boolean,
OUT blk_write_time float8,
OUT wal_records int8,
OUT wal_fpi int8,
- OUT wal_bytes numeric
+ OUT wal_bytes numeric,
+ OUT generic_calls int8
)
RETURNS SETOF record
AS 'MODULE_PATHNAME', 'pg_stat_statements_1_8'
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 62cccbfa44..f5801016d6 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -77,10 +77,12 @@
#include "storage/fd.h"
#include "storage/ipc.h"
#include "storage/spin.h"
+#include "tcop/pquery.h"
#include "tcop/utility.h"
#include "utils/acl.h"
#include "utils/builtins.h"
#include "utils/memutils.h"
+#include "utils/plancache.h"
#include "utils/timestamp.h"
PG_MODULE_MAGIC;
@@ -192,6 +194,7 @@ typedef struct Counters
int64 wal_records; /* # of WAL records generated */
int64 wal_fpi; /* # of WAL full page images generated */
uint64 wal_bytes; /* total amount of WAL generated in bytes */
+ int64 generic_calls; /* # of times generic plans executed */
} Counters;
/*
@@ -277,6 +280,10 @@ static int exec_nested_level = 0;
/* Current nesting depth of planner calls */
static int plan_nested_level = 0;
+/* Current and previous plan type */
+static bool is_plan_type_generic = false;
+static bool is_prev_plan_type_generic = false;
+
/* Saved hook values in case of unload */
static shmem_startup_hook_type prev_shmem_startup_hook = NULL;
static post_parse_analyze_hook_type prev_post_parse_analyze_hook = NULL;
@@ -1034,6 +1041,23 @@ pgss_ExecutorStart(QueryDesc *queryDesc, int eflags)
*/
if (pgss_enabled(exec_nested_level) && queryDesc->plannedstmt->queryId != UINT64CONST(0))
{
+ /*
+ * Since ActivePortal is not available at ExecutorEnd, we preserve
+ * the current and previous plan type here.
+ * Previous one is necessary since portals can be nested.
+ */
+ Assert(ActivePortal);
+
+ is_prev_plan_type_generic = is_plan_type_generic;
+
+ if (ActivePortal->cplan)
+ {
+ if (ActivePortal->cplan->is_generic)
+ is_plan_type_generic = true;
+ else
+ is_plan_type_generic = false;
+ }
+
/*
* Set up to track total elapsed time in ExecutorRun. Make sure the
* space is allocated in the per-query context so it will go away at
@@ -1122,6 +1146,9 @@ pgss_ExecutorEnd(QueryDesc *queryDesc)
NULL);
}
+ /* Storing has done. Set is_plan_type_generic back to the original. */
+ is_plan_type_generic = is_prev_plan_type_generic;
+
if (prev_ExecutorEnd)
prev_ExecutorEnd(queryDesc);
else
@@ -1427,6 +1454,8 @@ pgss_store(const char *query, uint64 queryId,
e->counters.max_time[kind] = total_time;
e->counters.mean_time[kind] = total_time;
}
+ else if (kind == PGSS_EXEC && is_plan_type_generic)
+ e->counters.generic_calls += 1;
else
{
/*
@@ -1510,8 +1539,8 @@ pg_stat_statements_reset(PG_FUNCTION_ARGS)
#define PG_STAT_STATEMENTS_COLS_V1_1 18
#define PG_STAT_STATEMENTS_COLS_V1_2 19
#define PG_STAT_STATEMENTS_COLS_V1_3 23
-#define PG_STAT_STATEMENTS_COLS_V1_8 32
-#define PG_STAT_STATEMENTS_COLS 32 /* maximum of above */
+#define PG_STAT_STATEMENTS_COLS_V1_8 33
+#define PG_STAT_STATEMENTS_COLS 33 /* maximum of above */
/*
* Retrieve statement statistics.
@@ -1863,6 +1892,7 @@ pg_stat_statements_internal(FunctionCallInfo fcinfo,
ObjectIdGetDatum(0),
Int32GetDatum(-1));
values[i++] = wal_bytes;
+ values[i++] = Int64GetDatumFast(tmp.generic_calls);
}
Assert(i == (api_version == PGSS_V1_0 ? PG_STAT_STATEMENTS_COLS_V1_0 :
diff --git a/doc/src/sgml/pgstatstatements.sgml b/doc/src/sgml/pgstatstatements.sgml
index 464bf0e5ae..8c5e304882 100644
--- a/doc/src/sgml/pgstatstatements.sgml
+++ b/doc/src/sgml/pgstatstatements.sgml
@@ -363,6 +363,14 @@
Total amount of WAL generated by the statement in bytes
</para></entry>
</row>
+ <row>
+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>generic_calls</structfield> <type>bigint</type>
+ </para>
+ <para>
+ Number of times the statement was executed as generic plan
+ </para></entry>
+ </row>
</tbody>
</tgroup>
</table>
diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/plancache.c
index 1a0950489d..b25c63ddfe 100644
--- a/src/backend/utils/cache/plancache.c
+++ b/src/backend/utils/cache/plancache.c
@@ -1219,10 +1219,12 @@ GetCachedPlan(CachedPlanSource *plansource, ParamListInfo boundParams,
plansource->total_custom_cost += cached_plan_cost(plan, true);
plansource->num_custom_plans++;
+ plan->is_generic = false;
}
else
{
plansource->num_generic_plans++;
+ plan->is_generic = true;
}
Assert(plan != NULL);
diff --git a/src/include/utils/plancache.h b/src/include/utils/plancache.h
index ff09c63a02..df9934d89b 100644
--- a/src/include/utils/plancache.h
+++ b/src/include/utils/plancache.h
@@ -158,6 +158,7 @@ typedef struct CachedPlan
int generation; /* parent's generation number for this plan */
int refcount; /* count of live references to this struct */
MemoryContext context; /* context containing this CachedPlan */
+ bool is_generic; /* is this plan generic or not? */
} CachedPlan;
/*
