public inbox for [email protected]help / color / mirror / Atom feed
[PATCH 4/7] Add explain(MACHINE) to hide machine-dependent output.. 13+ messages / 9 participants [nested] [flat]
* [PATCH 4/7] Add explain(MACHINE) to hide machine-dependent output.. @ 2020-02-23 00:45 Justin Pryzby <[email protected]> 0 siblings, 0 replies; 13+ messages in thread From: Justin Pryzby @ 2020-02-23 00:45 UTC (permalink / raw) This new option hides some output that has traditionally been shown; the option is enabled by regression mode to hide unstable output. This allows EXPLAIN ANALYZE to be used in regression tests with stable output. This is like a "quiet" mode, or negative verbosity. Also add regression tests for HashAgg and Bitmap scan, which previously had no tests with explain(analyze). This does not handle variations in "Workers Launched", or other parallel worker bits which are handled by force_parallel_mode=regress. Also add tests for show_hashagg_info and tidbitmap, for which there's no other test. --- src/backend/commands/explain.c | 77 +++++++++++++------ src/include/commands/explain.h | 1 + src/test/isolation/expected/horizons.out | 40 +++++----- src/test/isolation/specs/horizons.spec | 2 +- src/test/regress/expected/explain.out | 55 +++++++++++++ .../regress/expected/incremental_sort.out | 4 +- src/test/regress/expected/memoize.out | 35 ++++----- src/test/regress/expected/partition_prune.out | 4 +- src/test/regress/expected/select_parallel.out | 32 +++----- src/test/regress/expected/subselect.out | 21 +---- src/test/regress/sql/explain.sql | 17 ++++ src/test/regress/sql/memoize.sql | 4 +- src/test/regress/sql/select_parallel.sql | 20 +---- src/test/regress/sql/subselect.sql | 19 +---- 14 files changed, 182 insertions(+), 149 deletions(-) diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c index 6cc04f90fb4..8b4ff9624d4 100644 --- a/src/backend/commands/explain.c +++ b/src/backend/commands/explain.c @@ -175,6 +175,7 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt, bool summary_set = false; bool costs_set = false; bool buffers_set = false; + bool machine_set = false; /* Parse options list. */ foreach(lc, stmt->options) @@ -210,6 +211,11 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt, summary_set = true; es->summary = defGetBoolean(opt); } + else if (strcmp(opt->defname, "machine") == 0) + { + machine_set = true; + es->machine = defGetBoolean(opt); + } else if (strcmp(opt->defname, "format") == 0) { char *p = defGetString(opt); @@ -260,6 +266,9 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt, /* if the buffers option was not set explicitly, set default value */ es->buffers = (buffers_set) ? es->buffers : !explain_regress; + /* if the machine option was not set explicitly, set default value */ + es->machine = (machine_set) ? es->machine : !explain_regress; + query = castNode(Query, stmt->query); if (IsQueryIdEnabled()) jstate = JumbleQuery(query, pstate->p_sourcetext); @@ -621,7 +630,7 @@ ExplainOnePlan(PlannedStmt *plannedstmt, IntoClause *into, ExplainState *es, /* Create textual dump of plan tree */ ExplainPrintPlan(es, queryDesc); - if (es->verbose && plannedstmt->queryId != UINT64CONST(0)) + if (es->verbose && plannedstmt->queryId != UINT64CONST(0) && es->machine) { /* * Output the queryid as an int64 rather than a uint64 so we match @@ -632,7 +641,7 @@ ExplainOnePlan(PlannedStmt *plannedstmt, IntoClause *into, ExplainState *es, } /* Show buffer usage in planning */ - if (bufusage) + if (bufusage && es->buffers) { ExplainOpenGroup("Planning", "Planning", true, es); show_buffer_usage(es, bufusage, true); @@ -1772,7 +1781,7 @@ ExplainNode(PlanState *planstate, List *ancestors, if (plan->qual) show_instrumentation_count("Rows Removed by Filter", 1, planstate, es); - if (es->analyze) + if (es->analyze && es->machine) ExplainPropertyFloat("Heap Fetches", NULL, planstate->instrument->ntuples2, 0, es); break; @@ -2746,8 +2755,12 @@ show_sort_info(SortState *sortstate, ExplainState *es) if (es->format == EXPLAIN_FORMAT_TEXT) { ExplainIndentText(es); - appendStringInfo(es->str, "Sort Method: %s %s: " INT64_FORMAT "kB\n", - sortMethod, spaceType, spaceUsed); + appendStringInfo(es->str, "Sort Method: %s", + sortMethod); + if (es->machine) + appendStringInfo(es->str, " %s: " INT64_FORMAT "kB", + spaceType, spaceUsed); + appendStringInfoString(es->str, "\n"); } else { @@ -2791,8 +2804,12 @@ show_sort_info(SortState *sortstate, ExplainState *es) { ExplainIndentText(es); appendStringInfo(es->str, - "Sort Method: %s %s: " INT64_FORMAT "kB\n", - sortMethod, spaceType, spaceUsed); + "Sort Method: %s", + sortMethod); + if (es->machine) + appendStringInfo(es->str, " %s: " INT64_FORMAT "kB", spaceType, spaceUsed); + + appendStringInfoString(es->str, "\n"); } else { @@ -3080,25 +3097,26 @@ show_hash_info(HashState *hashstate, ExplainState *es) ExplainPropertyInteger("Peak Memory Usage", "kB", spacePeakKb, es); } - else if (hinstrument.nbatch_original != hinstrument.nbatch || - hinstrument.nbuckets_original != hinstrument.nbuckets) + else { ExplainIndentText(es); - appendStringInfo(es->str, - "Buckets: %d (originally %d) Batches: %d (originally %d) Memory Usage: %ldkB\n", + if (hinstrument.nbatch_original != hinstrument.nbatch || + hinstrument.nbuckets_original != hinstrument.nbuckets) + appendStringInfo(es->str, + "Buckets: %d (originally %d) Batches: %d (originally %d)", hinstrument.nbuckets, hinstrument.nbuckets_original, hinstrument.nbatch, - hinstrument.nbatch_original, - spacePeakKb); - } - else - { - ExplainIndentText(es); - appendStringInfo(es->str, - "Buckets: %d Batches: %d Memory Usage: %ldkB\n", - hinstrument.nbuckets, hinstrument.nbatch, - spacePeakKb); + hinstrument.nbatch_original); + else + appendStringInfo(es->str, + "Buckets: %d Batches: %d", + hinstrument.nbuckets, hinstrument.nbatch); + + if (es->machine) + appendStringInfo(es->str, " Memory Usage: %ldkB", spacePeakKb); + + appendStringInfoChar(es->str, '\n'); } } } @@ -3182,12 +3200,16 @@ show_memoize_info(MemoizeState *mstate, List *ancestors, ExplainState *es) { ExplainIndentText(es); appendStringInfo(es->str, - "Hits: " UINT64_FORMAT " Misses: " UINT64_FORMAT " Evictions: " UINT64_FORMAT " Overflows: " UINT64_FORMAT " Memory Usage: " INT64_FORMAT "kB\n", + "Hits: " UINT64_FORMAT " Misses: " UINT64_FORMAT " Evictions: " UINT64_FORMAT " Overflows: " UINT64_FORMAT, mstate->stats.cache_hits, mstate->stats.cache_misses, mstate->stats.cache_evictions, - mstate->stats.cache_overflows, + mstate->stats.cache_overflows); + if (es->machine) + appendStringInfo(es->str, " Memory Usage: " INT64_FORMAT "kB", memPeakKb); + + appendStringInfoChar(es->str, '\n'); } } @@ -3256,13 +3278,16 @@ show_hashagg_info(AggState *aggstate, ExplainState *es) Agg *agg = (Agg *) aggstate->ss.ps.plan; int64 memPeakKb = (aggstate->hash_mem_peak + 1023) / 1024; + /* XXX: there's nothing portable we can show here ? */ + if (!es->machine) + return; + if (agg->aggstrategy != AGG_HASHED && agg->aggstrategy != AGG_MIXED) return; if (es->format != EXPLAIN_FORMAT_TEXT) { - if (es->costs) ExplainPropertyInteger("Planned Partitions", NULL, aggstate->hash_planned_partitions, es); @@ -3375,6 +3400,10 @@ show_hashagg_info(AggState *aggstate, ExplainState *es) static void show_tidbitmap_info(BitmapHeapScanState *planstate, ExplainState *es) { + /* XXX: there's nothing portable we can show here ? */ + if (!es->machine) + return; + if (es->format != EXPLAIN_FORMAT_TEXT) { ExplainPropertyInteger("Exact Heap Blocks", NULL, diff --git a/src/include/commands/explain.h b/src/include/commands/explain.h index ce8c458d731..931dad08cbc 100644 --- a/src/include/commands/explain.h +++ b/src/include/commands/explain.h @@ -46,6 +46,7 @@ typedef struct ExplainState bool timing; /* print detailed node timing */ bool summary; /* print total planning and execution timing */ bool settings; /* print modified settings */ + bool machine; /* print memory/disk and other machine-specific output */ ExplainFormat format; /* output format */ /* state for output formatting --- not reset for each new plan tree */ int indent; /* current indentation level */ diff --git a/src/test/isolation/expected/horizons.out b/src/test/isolation/expected/horizons.out index 4150b2dee64..ee3e495a646 100644 --- a/src/test/isolation/expected/horizons.out +++ b/src/test/isolation/expected/horizons.out @@ -24,7 +24,7 @@ Index Only Scan using horizons_tst_data_key on horizons_tst step pruner_query: SELECT explain_json($$ - EXPLAIN (FORMAT json, BUFFERS, ANALYZE) + EXPLAIN (FORMAT json, BUFFERS, ANALYZE, machine) SELECT * FROM horizons_tst ORDER BY data;$$)->0->'Plan'->'Heap Fetches'; ?column? @@ -34,7 +34,7 @@ step pruner_query: step pruner_query: SELECT explain_json($$ - EXPLAIN (FORMAT json, BUFFERS, ANALYZE) + EXPLAIN (FORMAT json, BUFFERS, ANALYZE, machine) SELECT * FROM horizons_tst ORDER BY data;$$)->0->'Plan'->'Heap Fetches'; ?column? @@ -47,7 +47,7 @@ step pruner_delete: step pruner_query: SELECT explain_json($$ - EXPLAIN (FORMAT json, BUFFERS, ANALYZE) + EXPLAIN (FORMAT json, BUFFERS, ANALYZE, machine) SELECT * FROM horizons_tst ORDER BY data;$$)->0->'Plan'->'Heap Fetches'; ?column? @@ -57,7 +57,7 @@ step pruner_query: step pruner_query: SELECT explain_json($$ - EXPLAIN (FORMAT json, BUFFERS, ANALYZE) + EXPLAIN (FORMAT json, BUFFERS, ANALYZE, machine) SELECT * FROM horizons_tst ORDER BY data;$$)->0->'Plan'->'Heap Fetches'; ?column? @@ -94,7 +94,7 @@ Index Only Scan using horizons_tst_data_key on horizons_tst step pruner_query: SELECT explain_json($$ - EXPLAIN (FORMAT json, BUFFERS, ANALYZE) + EXPLAIN (FORMAT json, BUFFERS, ANALYZE, machine) SELECT * FROM horizons_tst ORDER BY data;$$)->0->'Plan'->'Heap Fetches'; ?column? @@ -104,7 +104,7 @@ step pruner_query: step pruner_query: SELECT explain_json($$ - EXPLAIN (FORMAT json, BUFFERS, ANALYZE) + EXPLAIN (FORMAT json, BUFFERS, ANALYZE, machine) SELECT * FROM horizons_tst ORDER BY data;$$)->0->'Plan'->'Heap Fetches'; ?column? @@ -117,7 +117,7 @@ step pruner_delete: step pruner_query: SELECT explain_json($$ - EXPLAIN (FORMAT json, BUFFERS, ANALYZE) + EXPLAIN (FORMAT json, BUFFERS, ANALYZE, machine) SELECT * FROM horizons_tst ORDER BY data;$$)->0->'Plan'->'Heap Fetches'; ?column? @@ -127,7 +127,7 @@ step pruner_query: step pruner_query: SELECT explain_json($$ - EXPLAIN (FORMAT json, BUFFERS, ANALYZE) + EXPLAIN (FORMAT json, BUFFERS, ANALYZE, machine) SELECT * FROM horizons_tst ORDER BY data;$$)->0->'Plan'->'Heap Fetches'; ?column? @@ -156,7 +156,7 @@ step ll_start: step pruner_query: SELECT explain_json($$ - EXPLAIN (FORMAT json, BUFFERS, ANALYZE) + EXPLAIN (FORMAT json, BUFFERS, ANALYZE, machine) SELECT * FROM horizons_tst ORDER BY data;$$)->0->'Plan'->'Heap Fetches'; ?column? @@ -166,7 +166,7 @@ step pruner_query: step pruner_query: SELECT explain_json($$ - EXPLAIN (FORMAT json, BUFFERS, ANALYZE) + EXPLAIN (FORMAT json, BUFFERS, ANALYZE, machine) SELECT * FROM horizons_tst ORDER BY data;$$)->0->'Plan'->'Heap Fetches'; ?column? @@ -180,7 +180,7 @@ step pruner_delete: step pruner_query: SELECT explain_json($$ - EXPLAIN (FORMAT json, BUFFERS, ANALYZE) + EXPLAIN (FORMAT json, BUFFERS, ANALYZE, machine) SELECT * FROM horizons_tst ORDER BY data;$$)->0->'Plan'->'Heap Fetches'; ?column? @@ -190,7 +190,7 @@ step pruner_query: step pruner_query: SELECT explain_json($$ - EXPLAIN (FORMAT json, BUFFERS, ANALYZE) + EXPLAIN (FORMAT json, BUFFERS, ANALYZE, machine) SELECT * FROM horizons_tst ORDER BY data;$$)->0->'Plan'->'Heap Fetches'; ?column? @@ -220,7 +220,7 @@ step ll_start: step pruner_query: SELECT explain_json($$ - EXPLAIN (FORMAT json, BUFFERS, ANALYZE) + EXPLAIN (FORMAT json, BUFFERS, ANALYZE, machine) SELECT * FROM horizons_tst ORDER BY data;$$)->0->'Plan'->'Heap Fetches'; ?column? @@ -230,7 +230,7 @@ step pruner_query: step pruner_query: SELECT explain_json($$ - EXPLAIN (FORMAT json, BUFFERS, ANALYZE) + EXPLAIN (FORMAT json, BUFFERS, ANALYZE, machine) SELECT * FROM horizons_tst ORDER BY data;$$)->0->'Plan'->'Heap Fetches'; ?column? @@ -246,7 +246,7 @@ step pruner_vacuum: step pruner_query: SELECT explain_json($$ - EXPLAIN (FORMAT json, BUFFERS, ANALYZE) + EXPLAIN (FORMAT json, BUFFERS, ANALYZE, machine) SELECT * FROM horizons_tst ORDER BY data;$$)->0->'Plan'->'Heap Fetches'; ?column? @@ -256,7 +256,7 @@ step pruner_query: step pruner_query: SELECT explain_json($$ - EXPLAIN (FORMAT json, BUFFERS, ANALYZE) + EXPLAIN (FORMAT json, BUFFERS, ANALYZE, machine) SELECT * FROM horizons_tst ORDER BY data;$$)->0->'Plan'->'Heap Fetches'; ?column? @@ -285,7 +285,7 @@ step ll_start: step pruner_query: SELECT explain_json($$ - EXPLAIN (FORMAT json, BUFFERS, ANALYZE) + EXPLAIN (FORMAT json, BUFFERS, ANALYZE, machine) SELECT * FROM horizons_tst ORDER BY data;$$)->0->'Plan'->'Heap Fetches'; ?column? @@ -295,7 +295,7 @@ step pruner_query: step pruner_query: SELECT explain_json($$ - EXPLAIN (FORMAT json, BUFFERS, ANALYZE) + EXPLAIN (FORMAT json, BUFFERS, ANALYZE, machine) SELECT * FROM horizons_tst ORDER BY data;$$)->0->'Plan'->'Heap Fetches'; ?column? @@ -311,7 +311,7 @@ step pruner_vacuum: step pruner_query: SELECT explain_json($$ - EXPLAIN (FORMAT json, BUFFERS, ANALYZE) + EXPLAIN (FORMAT json, BUFFERS, ANALYZE, machine) SELECT * FROM horizons_tst ORDER BY data;$$)->0->'Plan'->'Heap Fetches'; ?column? @@ -321,7 +321,7 @@ step pruner_query: step pruner_query: SELECT explain_json($$ - EXPLAIN (FORMAT json, BUFFERS, ANALYZE) + EXPLAIN (FORMAT json, BUFFERS, ANALYZE, machine) SELECT * FROM horizons_tst ORDER BY data;$$)->0->'Plan'->'Heap Fetches'; ?column? diff --git a/src/test/isolation/specs/horizons.spec b/src/test/isolation/specs/horizons.spec index d5239ff2287..082205d36ba 100644 --- a/src/test/isolation/specs/horizons.spec +++ b/src/test/isolation/specs/horizons.spec @@ -82,7 +82,7 @@ step pruner_vacuum step pruner_query { SELECT explain_json($$ - EXPLAIN (FORMAT json, BUFFERS, ANALYZE) + EXPLAIN (FORMAT json, BUFFERS, ANALYZE, machine) SELECT * FROM horizons_tst ORDER BY data;$$)->0->'Plan'->'Heap Fetches'; } diff --git a/src/test/regress/expected/explain.out b/src/test/regress/expected/explain.out index 188dc7ccec9..36c20e06809 100644 --- a/src/test/regress/expected/explain.out +++ b/src/test/regress/expected/explain.out @@ -276,6 +276,61 @@ select explain_filter('explain (buffers, format json) select * from int8_tbl i8' ] (1 row) +-- HashAgg +begin; +SET work_mem='64kB'; +select explain_filter('explain (analyze) SELECT a, COUNT(1) FROM generate_series(1,9999)a GROUP BY 1'); + explain_filter +---------------------------------------------------------------------------------------------------------------- + HashAggregate (cost=N.N..N.N rows=N width=N) (actual time=N.N..N.N rows=N loops=N) + Group Key: a + Batches: N Memory Usage: NkB Disk Usage: NkB + -> Function Scan on generate_series a (cost=N.N..N.N rows=N width=N) (actual time=N.N..N.N rows=N loops=N) + Planning Time: N.N ms + Execution Time: N.N ms +(6 rows) + +select explain_filter('explain (analyze, machine off) SELECT a, COUNT(1) FROM generate_series(1,9999)a GROUP BY 1'); + explain_filter +---------------------------------------------------------------------------------------------------------------- + HashAggregate (cost=N.N..N.N rows=N width=N) (actual time=N.N..N.N rows=N loops=N) + Group Key: a + -> Function Scan on generate_series a (cost=N.N..N.N rows=N width=N) (actual time=N.N..N.N rows=N loops=N) + Planning Time: N.N ms + Execution Time: N.N ms +(5 rows) + +rollback; +-- Bitmap scan +begin; +SET enable_indexscan=no; +CREATE TABLE explainbitmap AS SELECT i AS a FROM generate_series(1,999) AS i; +ANALYZE explainbitmap; +CREATE INDEX ON explainbitmap(a); +select explain_filter('explain (analyze) SELECT * FROM explainbitmap WHERE a<9'); + explain_filter +---------------------------------------------------------------------------------------------------------------------- + Bitmap Heap Scan on explainbitmap (cost=N.N..N.N rows=N width=N) (actual time=N.N..N.N rows=N loops=N) + Recheck Cond: (a < N) + Heap Blocks: exact=N + -> Bitmap Index Scan on explainbitmap_a_idx (cost=N.N..N.N rows=N width=N) (actual time=N.N..N.N rows=N loops=N) + Index Cond: (a < N) + Planning Time: N.N ms + Execution Time: N.N ms +(7 rows) + +select explain_filter('explain (analyze, machine off) SELECT * FROM explainbitmap WHERE a<9'); + explain_filter +---------------------------------------------------------------------------------------------------------------------- + Bitmap Heap Scan on explainbitmap (cost=N.N..N.N rows=N width=N) (actual time=N.N..N.N rows=N loops=N) + Recheck Cond: (a < N) + -> Bitmap Index Scan on explainbitmap_a_idx (cost=N.N..N.N rows=N width=N) (actual time=N.N..N.N rows=N loops=N) + Index Cond: (a < N) + Planning Time: N.N ms + Execution Time: N.N ms +(6 rows) + +rollback; -- SETTINGS option -- We have to ignore other settings that might be imposed by the environment, -- so printing the whole Settings field unfortunately won't do. diff --git a/src/test/regress/expected/incremental_sort.out b/src/test/regress/expected/incremental_sort.out index 545e301e482..56a32df5369 100644 --- a/src/test/regress/expected/incremental_sort.out +++ b/src/test/regress/expected/incremental_sort.out @@ -542,7 +542,7 @@ select explain_analyze_without_memory('select * from (select * from t order by a Full-sort Groups: 2 Sort Methods: top-N heapsort, quicksort Average Memory: NNkB Peak Memory: NNkB -> Sort (actual rows=101 loops=1) Sort Key: t.a - Sort Method: quicksort Memory: NNkB + Sort Method: quicksort -> Seq Scan on t (actual rows=1000 loops=1) (9 rows) @@ -745,7 +745,7 @@ select explain_analyze_without_memory('select * from (select * from t order by a Pre-sorted Groups: 5 Sort Methods: top-N heapsort, quicksort Average Memory: NNkB Peak Memory: NNkB -> Sort (actual rows=1000 loops=1) Sort Key: t.a - Sort Method: quicksort Memory: NNkB + Sort Method: quicksort -> Seq Scan on t (actual rows=1000 loops=1) (10 rows) diff --git a/src/test/regress/expected/memoize.out b/src/test/regress/expected/memoize.out index 4ca0bd1f1e1..fbe6bed4330 100644 --- a/src/test/regress/expected/memoize.out +++ b/src/test/regress/expected/memoize.out @@ -21,9 +21,7 @@ begin end if; ln := regexp_replace(ln, 'Evictions: 0', 'Evictions: Zero'); ln := regexp_replace(ln, 'Evictions: \d+', 'Evictions: N'); - ln := regexp_replace(ln, 'Memory Usage: \d+', 'Memory Usage: N'); - ln := regexp_replace(ln, 'Heap Fetches: \d+', 'Heap Fetches: N'); - ln := regexp_replace(ln, 'loops=\d+', 'loops=N'); + ln := regexp_replace(ln, 'loops=\d+', 'loops=N'); return next ln; end loop; end; @@ -45,11 +43,10 @@ WHERE t2.unique1 < 1000;', false); -> Memoize (actual rows=1 loops=N) Cache Key: t2.twenty Cache Mode: logical - Hits: 980 Misses: 20 Evictions: Zero Overflows: 0 Memory Usage: NkB + Hits: 980 Misses: 20 Evictions: Zero Overflows: 0 -> Index Only Scan using tenk1_unique1 on tenk1 t1 (actual rows=1 loops=N) Index Cond: (unique1 = t2.twenty) - Heap Fetches: N -(12 rows) +(11 rows) -- And check we get the expected results. SELECT COUNT(*),AVG(t1.unique1) FROM tenk1 t1 @@ -75,11 +72,10 @@ WHERE t1.unique1 < 1000;', false); -> Memoize (actual rows=1 loops=N) Cache Key: t1.twenty Cache Mode: logical - Hits: 980 Misses: 20 Evictions: Zero Overflows: 0 Memory Usage: NkB + Hits: 980 Misses: 20 Evictions: Zero Overflows: 0 -> Index Only Scan using tenk1_unique1 on tenk1 t2 (actual rows=1 loops=N) Index Cond: (unique1 = t1.twenty) - Heap Fetches: N -(12 rows) +(11 rows) -- And check we get the expected results. SELECT COUNT(*),AVG(t2.unique1) FROM tenk1 t1, @@ -110,11 +106,10 @@ WHERE t2.unique1 < 1200;', true); -> Memoize (actual rows=1 loops=N) Cache Key: t2.thousand Cache Mode: logical - Hits: N Misses: N Evictions: N Overflows: 0 Memory Usage: NkB + Hits: N Misses: N Evictions: N Overflows: 0 -> Index Only Scan using tenk1_unique1 on tenk1 t1 (actual rows=1 loops=N) Index Cond: (unique1 = t2.thousand) - Heap Fetches: N -(12 rows) +(11 rows) CREATE TABLE flt (f float); CREATE INDEX flt_f_idx ON flt (f); @@ -128,15 +123,13 @@ SELECT * FROM flt f1 INNER JOIN flt f2 ON f1.f = f2.f;', false); ------------------------------------------------------------------------------- Nested Loop (actual rows=4 loops=N) -> Index Only Scan using flt_f_idx on flt f1 (actual rows=2 loops=N) - Heap Fetches: N -> Memoize (actual rows=2 loops=N) Cache Key: f1.f Cache Mode: logical - Hits: 1 Misses: 1 Evictions: Zero Overflows: 0 Memory Usage: NkB + Hits: 1 Misses: 1 Evictions: Zero Overflows: 0 -> Index Only Scan using flt_f_idx on flt f2 (actual rows=2 loops=N) Index Cond: (f = f1.f) - Heap Fetches: N -(10 rows) +(8 rows) -- Ensure memoize operates in binary mode SELECT explain_memoize(' @@ -145,15 +138,13 @@ SELECT * FROM flt f1 INNER JOIN flt f2 ON f1.f >= f2.f;', false); ------------------------------------------------------------------------------- Nested Loop (actual rows=4 loops=N) -> Index Only Scan using flt_f_idx on flt f1 (actual rows=2 loops=N) - Heap Fetches: N -> Memoize (actual rows=2 loops=N) Cache Key: f1.f Cache Mode: binary - Hits: 0 Misses: 2 Evictions: Zero Overflows: 0 Memory Usage: NkB + Hits: 0 Misses: 2 Evictions: Zero Overflows: 0 -> Index Only Scan using flt_f_idx on flt f2 (actual rows=2 loops=N) Index Cond: (f <= f1.f) - Heap Fetches: N -(10 rows) +(8 rows) DROP TABLE flt; -- Exercise Memoize in binary mode with a large fixed width type and a @@ -175,7 +166,7 @@ SELECT * FROM strtest s1 INNER JOIN strtest s2 ON s1.n >= s2.n;', false); -> Memoize (actual rows=4 loops=N) Cache Key: s1.n Cache Mode: binary - Hits: 3 Misses: 3 Evictions: Zero Overflows: 0 Memory Usage: NkB + Hits: 3 Misses: 3 Evictions: Zero Overflows: 0 -> Index Scan using strtest_n_idx on strtest s2 (actual rows=4 loops=N) Index Cond: (n <= s1.n) (8 rows) @@ -190,7 +181,7 @@ SELECT * FROM strtest s1 INNER JOIN strtest s2 ON s1.t >= s2.t;', false); -> Memoize (actual rows=4 loops=N) Cache Key: s1.t Cache Mode: binary - Hits: 3 Misses: 3 Evictions: Zero Overflows: 0 Memory Usage: NkB + Hits: 3 Misses: 3 Evictions: Zero Overflows: 0 -> Index Scan using strtest_t_idx on strtest s2 (actual rows=4 loops=N) Index Cond: (t <= s1.t) (8 rows) diff --git a/src/test/regress/expected/partition_prune.out b/src/test/regress/expected/partition_prune.out index 7555764c779..cabadd48b81 100644 --- a/src/test/regress/expected/partition_prune.out +++ b/src/test/regress/expected/partition_prune.out @@ -2479,7 +2479,6 @@ update ab_a1 set b = 3 from ab where ab.a = 1 and ab.a = ab_a1.a; Index Cond: (a = 1) -> Bitmap Heap Scan on ab_a1_b2 ab_a1_2 (actual rows=1 loops=1) Recheck Cond: (a = 1) - Heap Blocks: exact=1 -> Bitmap Index Scan on ab_a1_b2_a_idx (actual rows=1 loops=1) Index Cond: (a = 1) -> Bitmap Heap Scan on ab_a1_b3 ab_a1_3 (actual rows=0 loops=1) @@ -2494,14 +2493,13 @@ update ab_a1 set b = 3 from ab where ab.a = 1 and ab.a = ab_a1.a; Index Cond: (a = 1) -> Bitmap Heap Scan on ab_a1_b2 ab_2 (actual rows=1 loops=1) Recheck Cond: (a = 1) - Heap Blocks: exact=1 -> Bitmap Index Scan on ab_a1_b2_a_idx (actual rows=1 loops=1) Index Cond: (a = 1) -> Bitmap Heap Scan on ab_a1_b3 ab_3 (actual rows=0 loops=1) Recheck Cond: (a = 1) -> Bitmap Index Scan on ab_a1_b3_a_idx (actual rows=1 loops=1) Index Cond: (a = 1) -(34 rows) +(32 rows) table ab; a | b diff --git a/src/test/regress/expected/select_parallel.out b/src/test/regress/expected/select_parallel.out index 4ea1aa7dfd4..9f36c627419 100644 --- a/src/test/regress/expected/select_parallel.out +++ b/src/test/regress/expected/select_parallel.out @@ -562,24 +562,11 @@ explain (analyze, timing off, summary off, costs off) alter table tenk2 reset (parallel_workers); reset work_mem; -create function explain_parallel_sort_stats() returns setof text -language plpgsql as -$$ -declare ln text; -begin - for ln in - explain (analyze, timing off, summary off, costs off) - select * from +explain (analyze) +select * from (select ten from tenk1 where ten < 100 order by ten) ss - right join (values (1),(2),(3)) v(x) on true - loop - ln := regexp_replace(ln, 'Memory: \S*', 'Memory: xxx'); - return next ln; - end loop; -end; -$$; -select * from explain_parallel_sort_stats(); - explain_parallel_sort_stats + right join (values (1),(2),(3)) v(x) on true; + QUERY PLAN -------------------------------------------------------------------------- Nested Loop Left Join (actual rows=30000 loops=1) -> Values Scan on "*VALUES*" (actual rows=3 loops=1) @@ -588,11 +575,11 @@ select * from explain_parallel_sort_stats(); Workers Launched: 4 -> Sort (actual rows=2000 loops=15) Sort Key: tenk1.ten - Sort Method: quicksort Memory: xxx - Worker 0: Sort Method: quicksort Memory: xxx - Worker 1: Sort Method: quicksort Memory: xxx - Worker 2: Sort Method: quicksort Memory: xxx - Worker 3: Sort Method: quicksort Memory: xxx + Sort Method: quicksort + Worker 0: Sort Method: quicksort + Worker 1: Sort Method: quicksort + Worker 2: Sort Method: quicksort + Worker 3: Sort Method: quicksort -> Parallel Seq Scan on tenk1 (actual rows=2000 loops=15) Filter: (ten < 100) (14 rows) @@ -603,7 +590,6 @@ reset enable_mergejoin; reset enable_material; reset effective_io_concurrency; drop table bmscantest; -drop function explain_parallel_sort_stats(); -- test parallel merge join path. set enable_hashjoin to off; set enable_nestloop to off; diff --git a/src/test/regress/expected/subselect.out b/src/test/regress/expected/subselect.out index 45c75eecc5f..527f04e122b 100644 --- a/src/test/regress/expected/subselect.out +++ b/src/test/regress/expected/subselect.out @@ -1550,27 +1550,15 @@ insert into sq_limit values (6, 2, 2), (7, 3, 3), (8, 4, 4); -create function explain_sq_limit() returns setof text language plpgsql as -$$ -declare ln text; -begin - for ln in - explain (analyze, summary off, timing off, costs off) - select * from (select pk,c2 from sq_limit order by c1,pk) as x limit 3 - loop - ln := regexp_replace(ln, 'Memory: \S*', 'Memory: xxx'); - return next ln; - end loop; -end; -$$; -select * from explain_sq_limit(); - explain_sq_limit +explain (analyze) + select * from (select pk,c2 from sq_limit order by c1,pk) as x limit 3; + QUERY PLAN ---------------------------------------------------------------- Limit (actual rows=3 loops=1) -> Subquery Scan on x (actual rows=3 loops=1) -> Sort (actual rows=3 loops=1) Sort Key: sq_limit.c1, sq_limit.pk - Sort Method: top-N heapsort Memory: xxx + Sort Method: top-N heapsort -> Seq Scan on sq_limit (actual rows=8 loops=1) (6 rows) @@ -1582,7 +1570,6 @@ select * from (select pk,c2 from sq_limit order by c1,pk) as x limit 3; 2 | 2 (3 rows) -drop function explain_sq_limit(); drop table sq_limit; -- -- Ensure that backward scan direction isn't propagated into diff --git a/src/test/regress/sql/explain.sql b/src/test/regress/sql/explain.sql index f91d1004d5e..c38857d3461 100644 --- a/src/test/regress/sql/explain.sql +++ b/src/test/regress/sql/explain.sql @@ -72,6 +72,23 @@ select explain_filter('explain (analyze, buffers, format yaml) select * from int select explain_filter('explain (buffers, format text) select * from int8_tbl i8'); select explain_filter('explain (buffers, format json) select * from int8_tbl i8'); +-- HashAgg +begin; +SET work_mem='64kB'; +select explain_filter('explain (analyze) SELECT a, COUNT(1) FROM generate_series(1,9999)a GROUP BY 1'); +select explain_filter('explain (analyze, machine off) SELECT a, COUNT(1) FROM generate_series(1,9999)a GROUP BY 1'); +rollback; + +-- Bitmap scan +begin; +SET enable_indexscan=no; +CREATE TABLE explainbitmap AS SELECT i AS a FROM generate_series(1,999) AS i; +ANALYZE explainbitmap; +CREATE INDEX ON explainbitmap(a); +select explain_filter('explain (analyze) SELECT * FROM explainbitmap WHERE a<9'); +select explain_filter('explain (analyze, machine off) SELECT * FROM explainbitmap WHERE a<9'); +rollback; + -- SETTINGS option -- We have to ignore other settings that might be imposed by the environment, -- so printing the whole Settings field unfortunately won't do. diff --git a/src/test/regress/sql/memoize.sql b/src/test/regress/sql/memoize.sql index c6ed5a2aa66..1368f8dd185 100644 --- a/src/test/regress/sql/memoize.sql +++ b/src/test/regress/sql/memoize.sql @@ -22,9 +22,7 @@ begin end if; ln := regexp_replace(ln, 'Evictions: 0', 'Evictions: Zero'); ln := regexp_replace(ln, 'Evictions: \d+', 'Evictions: N'); - ln := regexp_replace(ln, 'Memory Usage: \d+', 'Memory Usage: N'); - ln := regexp_replace(ln, 'Heap Fetches: \d+', 'Heap Fetches: N'); - ln := regexp_replace(ln, 'loops=\d+', 'loops=N'); + ln := regexp_replace(ln, 'loops=\d+', 'loops=N'); return next ln; end loop; end; diff --git a/src/test/regress/sql/select_parallel.sql b/src/test/regress/sql/select_parallel.sql index f9247312484..fc586161235 100644 --- a/src/test/regress/sql/select_parallel.sql +++ b/src/test/regress/sql/select_parallel.sql @@ -221,23 +221,10 @@ explain (analyze, timing off, summary off, costs off) alter table tenk2 reset (parallel_workers); reset work_mem; -create function explain_parallel_sort_stats() returns setof text -language plpgsql as -$$ -declare ln text; -begin - for ln in - explain (analyze, timing off, summary off, costs off) - select * from +explain (analyze) +select * from (select ten from tenk1 where ten < 100 order by ten) ss - right join (values (1),(2),(3)) v(x) on true - loop - ln := regexp_replace(ln, 'Memory: \S*', 'Memory: xxx'); - return next ln; - end loop; -end; -$$; -select * from explain_parallel_sort_stats(); + right join (values (1),(2),(3)) v(x) on true; reset enable_indexscan; reset enable_hashjoin; @@ -245,7 +232,6 @@ reset enable_mergejoin; reset enable_material; reset effective_io_concurrency; drop table bmscantest; -drop function explain_parallel_sort_stats(); -- test parallel merge join path. set enable_hashjoin to off; diff --git a/src/test/regress/sql/subselect.sql b/src/test/regress/sql/subselect.sql index 94ba91f5bb3..2311c9c0ed8 100644 --- a/src/test/regress/sql/subselect.sql +++ b/src/test/regress/sql/subselect.sql @@ -807,26 +807,11 @@ insert into sq_limit values (7, 3, 3), (8, 4, 4); -create function explain_sq_limit() returns setof text language plpgsql as -$$ -declare ln text; -begin - for ln in - explain (analyze, summary off, timing off, costs off) - select * from (select pk,c2 from sq_limit order by c1,pk) as x limit 3 - loop - ln := regexp_replace(ln, 'Memory: \S*', 'Memory: xxx'); - return next ln; - end loop; -end; -$$; - -select * from explain_sq_limit(); +explain (analyze) + select * from (select pk,c2 from sq_limit order by c1,pk) as x limit 3; select * from (select pk,c2 from sq_limit order by c1,pk) as x limit 3; -drop function explain_sq_limit(); - drop table sq_limit; -- -- 2.17.1 --ELVYuRnMxQ5nnKRy Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="0005-f-Rows-removed-by-filter.patch" ^ permalink raw reply [nested|flat] 13+ messages in thread
* pgsql: Add function to get memory context stats for processes @ 2025-04-08 09:10 Daniel Gustafsson <[email protected]> 0 siblings, 2 replies; 13+ messages in thread From: Daniel Gustafsson @ 2025-04-08 09:10 UTC (permalink / raw) To: [email protected] Add function to get memory context stats for processes This adds a function for retrieving memory context statistics and information from backends as well as auxiliary processes. The intended usecase is cluster debugging when under memory pressure or unanticipated memory usage characteristics. When calling the function it sends a signal to the specified process to submit statistics regarding its memory contexts into dynamic shared memory. Each memory context is returned in detail, followed by a cumulative total in case the number of contexts exceed the max allocated amount of shared memory. Each process is limited to use at most 1Mb memory for this. A summary can also be explicitly requested by the user, this will return the TopMemoryContext and a cumulative total of all lower contexts. In order to not block on busy processes the caller specifies the number of seconds during which to retry before timing out. In the case where no statistics are published within the set timeout, the last known statistics are returned, or NULL if no previously published statistics exist. This allows dash- board type queries to continually publish even if the target process is temporarily congested. Context records contain a timestamp to indicate when they were submitted. Author: Rahila Syed <[email protected]> Reviewed-by: Daniel Gustafsson <[email protected]> Reviewed-by: Andres Freund <[email protected]> Reviewed-by: Tomas Vondra <[email protected]> Reviewed-by: Atsushi Torikoshi <[email protected]> Reviewed-by: Fujii Masao <[email protected]> Reviewed-by: Alexander Korotkov <[email protected]> Discussion: https://postgr.es/m/CAH2L28v8mc9HDt8QoSJ8TRmKau_8FM_HKS41NeO9-6ZAkuZKXw@mail.gmail.com Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/042a66291b04f473cbc72f95f07438abd75ae3a9 Modified Files -------------- doc/src/sgml/func.sgml | 172 +++++++ src/backend/catalog/system_views.sql | 5 + src/backend/postmaster/autovacuum.c | 4 + src/backend/postmaster/checkpointer.c | 4 + src/backend/postmaster/interrupt.c | 4 + src/backend/postmaster/pgarch.c | 4 + src/backend/postmaster/startup.c | 4 + src/backend/postmaster/walsummarizer.c | 4 + src/backend/storage/ipc/ipci.c | 3 + src/backend/storage/ipc/procsignal.c | 3 + src/backend/storage/lmgr/lwlock.c | 2 + src/backend/storage/lmgr/proc.c | 1 + src/backend/tcop/postgres.c | 3 + src/backend/utils/activity/wait_event_names.txt | 1 + src/backend/utils/adt/mcxtfuncs.c | 426 ++++++++++++++-- src/backend/utils/init/globals.c | 1 + src/backend/utils/init/postinit.c | 7 + src/backend/utils/mmgr/mcxt.c | 645 +++++++++++++++++++++++- src/include/catalog/pg_proc.dat | 10 + src/include/miscadmin.h | 1 + src/include/storage/lwlock.h | 2 + src/include/storage/procsignal.h | 1 + src/include/utils/memutils.h | 82 +++ src/test/regress/expected/sysviews.out | 19 + src/test/regress/sql/sysviews.sql | 18 + src/tools/pgindent/typedefs.list | 4 + 26 files changed, 1385 insertions(+), 45 deletions(-) ^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: pgsql: Add function to get memory context stats for processes @ 2025-04-10 11:42 Laurenz Albe <[email protected]> parent: Daniel Gustafsson <[email protected]> 1 sibling, 2 replies; 13+ messages in thread From: Laurenz Albe @ 2025-04-10 11:42 UTC (permalink / raw) To: Daniel Gustafsson <[email protected]>; [email protected] On Tue, 2025-04-08 at 09:10 +0000, Daniel Gustafsson wrote: > Add function to get memory context stats for processes > > This adds a function for retrieving memory context statistics > and information from backends as well as auxiliary processes. > The intended usecase is cluster debugging when under memory > pressure or unanticipated memory usage characteristics. > > Discussion: https://postgr.es/m/CAH2L28v8mc9HDt8QoSJ8TRmKau_8FM_HKS41NeO9-6ZAkuZKXw@mail.gmail.com > > Details > ------- > https://git.postgresql.org/pg/commitdiff/042a66291b04f473cbc72f95f07438abd75ae3a9 > > [from the patch:] > diff --git a/src/include/storage/procsignal.h b/src/include/storage/procsignal.h > index 016dfd9b3f6..cfe14631445 100644 > --- a/src/include/storage/procsignal.h > +++ b/src/include/storage/procsignal.h > [...] > +extern dsa_area *area; This commit causes problems for PostGIS, because the name "area" collides with a PostGIS object: postgis_legacy.c:58:28: error: ‘area’ redeclared as different kind of symbol 58 | POSTGIS_DEPRECATE("3.0.0", area) | ^~~~ postgis_legacy.c:40:15: note: in definition of macro ‘POSTGIS_DEPRECATE’ 40 | Datum funcname(PG_FUNCTION_ARGS); \ | ^~~~~~~~ In file included from ../libpgcommon/lwgeom_pg.h:24, from postgis_legacy.c:37: /home/laurenz/pg/include/postgresql/server/utils/memutils.h:403:18: note: previous declaration of ‘area’ with type ‘dsa_area *’ 403 | extern dsa_area *area; | ^~~~ Now one can take the position that PostGIS as dependent library hs to adapt, but I think "area" is too generic a name. Could you envision renaming the global variable to something like "shm_area"? Attached is a patch for this change. I am not wedded to the name at all, it was just the first thing that popped into my head. Yours, Laurenz Albe Attachments: [text/x-patch] v1-0001-Rename-a-global-variable-to-avoid-name-collisions.patch (8.4K, ../../[email protected]/2-v1-0001-Rename-a-global-variable-to-avoid-name-collisions.patch) download | inline diff: From 9a207d76d4b1bbe5904bf346c6621324a9dd7359 Mon Sep 17 00:00:00 2001 From: Laurenz Albe <[email protected]> Date: Thu, 10 Apr 2025 13:38:10 +0200 Subject: [PATCH v1] Rename a global variable to avoid name collisions "area", introduced by 042a66291b04, is a very generic name, and indeed it collides with an object defined by PostGIS. While it is arguably the job of the dependent library to avoid name collisions, using a generic name like that is just an invitation for problems of that kind. Fix by renaming the variable to "shm_area". --- src/backend/utils/adt/mcxtfuncs.c | 14 +++++------ src/backend/utils/mmgr/mcxt.c | 40 +++++++++++++++---------------- src/include/utils/memutils.h | 2 +- 3 files changed, 28 insertions(+), 28 deletions(-) diff --git a/src/backend/utils/adt/mcxtfuncs.c b/src/backend/utils/adt/mcxtfuncs.c index 3ede88e5036..154ea38fdc2 100644 --- a/src/backend/utils/adt/mcxtfuncs.c +++ b/src/backend/utils/adt/mcxtfuncs.c @@ -533,21 +533,21 @@ pg_get_process_memory_contexts(PG_FUNCTION_ARGS) */ Assert(memCxtArea->memstats_dsa_handle != DSA_HANDLE_INVALID); /* Attach to the dsa area if we have not already done so */ - if (area == NULL) + if (shm_area == NULL) { MemoryContext oldcontext = CurrentMemoryContext; MemoryContextSwitchTo(TopMemoryContext); - area = dsa_attach(memCxtArea->memstats_dsa_handle); + shm_area = dsa_attach(memCxtArea->memstats_dsa_handle); MemoryContextSwitchTo(oldcontext); - dsa_pin_mapping(area); + dsa_pin_mapping(shm_area); } /* * Backend has finished publishing the stats, project them. */ memcxt_info = (MemoryStatsEntry *) - dsa_get_address(area, memCxtState[procNumber].memstats_dsa_pointer); + dsa_get_address(shm_area, memCxtState[procNumber].memstats_dsa_pointer); #define PG_GET_PROCESS_MEMORY_CONTEXTS_COLS 12 for (int i = 0; i < memCxtState[procNumber].total_stats; i++) @@ -566,7 +566,7 @@ pg_get_process_memory_contexts(PG_FUNCTION_ARGS) if (DsaPointerIsValid(memcxt_info[i].name)) { - name = (char *) dsa_get_address(area, memcxt_info[i].name); + name = (char *) dsa_get_address(shm_area, memcxt_info[i].name); values[0] = CStringGetTextDatum(name); } else @@ -574,7 +574,7 @@ pg_get_process_memory_contexts(PG_FUNCTION_ARGS) if (DsaPointerIsValid(memcxt_info[i].ident)) { - ident = (char *) dsa_get_address(area, memcxt_info[i].ident); + ident = (char *) dsa_get_address(shm_area, memcxt_info[i].ident); values[1] = CStringGetTextDatum(ident); } else @@ -586,7 +586,7 @@ pg_get_process_memory_contexts(PG_FUNCTION_ARGS) path_datum = (Datum *) palloc(path_length * sizeof(Datum)); if (DsaPointerIsValid(memcxt_info[i].path)) { - path_int = (int *) dsa_get_address(area, memcxt_info[i].path); + path_int = (int *) dsa_get_address(shm_area, memcxt_info[i].path); for (int j = 0; j < path_length; j++) path_datum[j] = Int32GetDatum(path_int[j]); path_array = construct_array_builtin(path_datum, path_length, INT4OID); diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c index cf4e22bf1cc..f32ed7967d9 100644 --- a/src/backend/utils/mmgr/mcxt.c +++ b/src/backend/utils/mmgr/mcxt.c @@ -172,7 +172,7 @@ MemoryContext CurTransactionContext = NULL; /* This is a transient link to the active portal's memory context: */ MemoryContext PortalContext = NULL; -dsa_area *area = NULL; +dsa_area *shm_area = NULL; static void MemoryContextDeleteOnly(MemoryContext context); static void MemoryContextCallResetCallbacks(MemoryContext context); @@ -1499,19 +1499,19 @@ ProcessGetMemoryContextInterrupt(void) MemoryContextSwitchTo(TopMemoryContext); - area = dsa_create(memCxtArea->lw_lock.tranche); + shm_area = dsa_create(memCxtArea->lw_lock.tranche); - handle = dsa_get_handle(area); + handle = dsa_get_handle(shm_area); MemoryContextSwitchTo(oldcontext); - dsa_pin_mapping(area); + dsa_pin_mapping(shm_area); /* * Pin the DSA area, this is to make sure the area remains attachable * even if current backend exits. This is done so that the statistics * are published even if the process exits while a client is waiting. */ - dsa_pin(area); + dsa_pin(shm_area); /* Set the handle in shared memory */ memCxtArea->memstats_dsa_handle = handle; @@ -1521,14 +1521,14 @@ ProcessGetMemoryContextInterrupt(void) * If DSA exists, created by another process publishing statistics, attach * to it. */ - else if (area == NULL) + else if (shm_area == NULL) { MemoryContext oldcontext = CurrentMemoryContext; MemoryContextSwitchTo(TopMemoryContext); - area = dsa_attach(memCxtArea->memstats_dsa_handle); + shm_area = dsa_attach(memCxtArea->memstats_dsa_handle); MemoryContextSwitchTo(oldcontext); - dsa_pin_mapping(area); + dsa_pin_mapping(shm_area); } LWLockRelease(&memCxtArea->lw_lock); @@ -1545,7 +1545,7 @@ ProcessGetMemoryContextInterrupt(void) * Free any previous allocations, free the name, ident and path * pointers before freeing the pointer that contains them. */ - free_memorycontextstate_dsa(area, memCxtState[idx].total_stats, + free_memorycontextstate_dsa(shm_area, memCxtState[idx].total_stats, memCxtState[idx].memstats_dsa_pointer); } @@ -1556,10 +1556,10 @@ ProcessGetMemoryContextInterrupt(void) */ memCxtState[idx].total_stats = stats_num; memCxtState[idx].memstats_dsa_pointer = - dsa_allocate0(area, stats_num * sizeof(MemoryStatsEntry)); + dsa_allocate0(shm_area, stats_num * sizeof(MemoryStatsEntry)); meminfo = (MemoryStatsEntry *) - dsa_get_address(area, memCxtState[idx].memstats_dsa_pointer); + dsa_get_address(shm_area, memCxtState[idx].memstats_dsa_pointer); if (summary) { @@ -1572,7 +1572,7 @@ ProcessGetMemoryContextInterrupt(void) &stat, true); path = lcons_int(1, path); PublishMemoryContext(meminfo, cxt_id, TopMemoryContext, path, stat, - 1, area, 100); + 1, shm_area, 100); cxt_id = cxt_id + 1; /* @@ -1602,7 +1602,7 @@ ProcessGetMemoryContextInterrupt(void) */ memCxtState[idx].total_stats = cxt_id++; PublishMemoryContext(meminfo, cxt_id, c, path, - grand_totals, num_contexts, area, 100); + grand_totals, num_contexts, shm_area, 100); } memCxtState[idx].total_stats = cxt_id; @@ -1632,7 +1632,7 @@ ProcessGetMemoryContextInterrupt(void) if (context_id < (max_stats - 1) || stats_count <= max_stats) { /* Copy statistics to DSA memory */ - PublishMemoryContext(meminfo, context_id, cur, path, stat, 1, area, 100); + PublishMemoryContext(meminfo, context_id, cur, path, stat, 1, shm_area, 100); } else { @@ -1657,8 +1657,8 @@ ProcessGetMemoryContextInterrupt(void) int namelen = strlen("Remaining Totals"); num_individual_stats = context_id + 1; - meminfo[max_stats - 1].name = dsa_allocate(area, namelen + 1); - nameptr = dsa_get_address(area, meminfo[max_stats - 1].name); + meminfo[max_stats - 1].name = dsa_allocate(shm_area, namelen + 1); + nameptr = dsa_get_address(shm_area, meminfo[max_stats - 1].name); strncpy(nameptr, "Remaining Totals", namelen); meminfo[max_stats - 1].ident = InvalidDsaPointer; meminfo[max_stats - 1].path = InvalidDsaPointer; @@ -1921,18 +1921,18 @@ AtProcExit_memstats_cleanup(int code, Datum arg) } /* If the dsa mapping could not be found, attach to the area */ - if (area == NULL) - area = dsa_attach(memCxtArea->memstats_dsa_handle); + if (shm_area == NULL) + shm_area = dsa_attach(memCxtArea->memstats_dsa_handle); /* * Free the memory context statistics, free the name, ident and path * pointers before freeing the pointer that contains these pointers and * integer statistics. */ - free_memorycontextstate_dsa(area, memCxtState[idx].total_stats, + free_memorycontextstate_dsa(shm_area, memCxtState[idx].total_stats, memCxtState[idx].memstats_dsa_pointer); - dsa_detach(area); + dsa_detach(shm_area); LWLockRelease(&memCxtState[idx].lw_lock); } diff --git a/src/include/utils/memutils.h b/src/include/utils/memutils.h index d328270fafc..79fb4302139 100644 --- a/src/include/utils/memutils.h +++ b/src/include/utils/memutils.h @@ -400,5 +400,5 @@ extern void HandleGetMemoryContextInterrupt(void); extern Size MemoryContextReportingShmemSize(void); extern void MemoryContextReportingShmemInit(void); extern void AtProcExit_memstats_cleanup(int code, Datum arg); -extern dsa_area *area; +extern dsa_area *shm_area; #endif /* MEMUTILS_H */ -- 2.49.0 ^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: pgsql: Add function to get memory context stats for processes @ 2025-04-10 11:48 Aleksander Alekseev <[email protected]> parent: Laurenz Albe <[email protected]> 1 sibling, 0 replies; 13+ messages in thread From: Aleksander Alekseev @ 2025-04-10 11:48 UTC (permalink / raw) To: [email protected]; +Cc: Laurenz Albe <[email protected]>; Daniel Gustafsson <[email protected]> Hi, > Now one can take the position that PostGIS as dependent library hs to > adapt, but I think "area" is too generic a name. Could you envision > renaming the global variable to something like "shm_area"? > > Attached is a patch for this change. > I am not wedded to the name at all, it was just the first thing that > popped into my head. I agree that the name is too generic for an exported symbol. -- Best regards, Aleksander Alekseev ^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: pgsql: Add function to get memory context stats for processes @ 2025-04-10 12:46 Daniel Gustafsson <[email protected]> parent: Laurenz Albe <[email protected]> 1 sibling, 0 replies; 13+ messages in thread From: Daniel Gustafsson @ 2025-04-10 12:46 UTC (permalink / raw) To: Laurenz Albe <[email protected]>; +Cc: [email protected] > On 10 Apr 2025, at 13:42, Laurenz Albe <[email protected]> wrote: > On Tue, 2025-04-08 at 09:10 +0000, Daniel Gustafsson wrote: >> +extern dsa_area *area; > > This commit causes problems for PostGIS, because the name "area" collides > with a PostGIS object: Thanks for the report, I've already posted a patch [0] and will push that shortly. -- Daniel Gustafsson [0] https://postgr.es/m/[email protected] ^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: pgsql: Add function to get memory context stats for processes @ 2025-04-10 20:05 Andres Freund <[email protected]> parent: Daniel Gustafsson <[email protected]> 1 sibling, 1 reply; 13+ messages in thread From: Andres Freund @ 2025-04-10 20:05 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: Daniel Gustafsson <[email protected]>; PostgreSQL Hackers <[email protected]>; Rahila Syed <[email protected]> Hi, On 2025-04-10 09:31:00 -0400, Robert Haas wrote: > On Tue, Apr 8, 2025 at 5:10 AM Daniel Gustafsson > <[email protected]> wrote: > > Add function to get memory context stats for processes > > Apologies if this has already been discussed, but what is the argument > that it is safe to do everything in ProcessGetMemoryContextInterrupt() > at an arbitrary CHECK_FOR_INTERRUPTS() call? We have > CHECK_FOR_INTERRUPTS() calls in some quite low-level places, such as > walkdir() and copydir(). I don't think there's any guarantee that it's > safe to perform DSA operations at an arbitrary place where > CHECK_FOR_INTERRUPTS() is called, and I'm not even quite sure that > it's safe to assume that the local memory-context tree is in a > consistent state when CHECK_FOR_INTERRUPTS() is called. I don't know of existing discussion, but it seems rather fundamental to me - if either DSA or memory contexts could be inconsistent at a CFI(), how could it possibly be safe to interrupt at that point? After all, after an error you need to be able to reset the memory contexts / release memory in a dsa/dshash/whatnot? Memory context reset requires walking over the allocations made in the context, similar releasing a dsa? Greetings, Andres Freund ^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: pgsql: Add function to get memory context stats for processes @ 2025-04-14 14:13 Andres Freund <[email protected]> parent: Andres Freund <[email protected]> 0 siblings, 2 replies; 13+ messages in thread From: Andres Freund @ 2025-04-14 14:13 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: Daniel Gustafsson <[email protected]>; PostgreSQL Hackers <[email protected]>; Rahila Syed <[email protected]> Hi, On 2025-04-14 10:03:28 -0400, Robert Haas wrote: > On Thu, Apr 10, 2025 at 4:05 PM Andres Freund <[email protected]> wrote: > > I don't know of existing discussion, but it seems rather fundamental to me - > > if either DSA or memory contexts could be inconsistent at a CFI(), how could > > it possibly be safe to interrupt at that point? After all, after an error you > > need to be able to reset the memory contexts / release memory in a > > dsa/dshash/whatnot? Memory context reset requires walking over the allocations > > made in the context, similar releasing a dsa? > > I think it would be a bit surprising if somebody put a > CHECK_FOR_INTERRUPTS() inside aset.c or similar, but I don't see a > reason why we couldn't end up with one reachable via the DSA code. DSA > calls DSM which depending on dynamic_shared_memory_type might involve > filesystem operations. That's a fairly large amount of code. I admit I > have no particular theory about how CFI could be reachable from there > today, but even if it definitely isn't, I don't see why someone would > hesitate to add one in the future. There very well could be a CFI - but it better be somewhere where the in-memory state is consistent. Otherwise an error inside raised in the CFI would lead the in-memory state inconsistent which then would cause problems when cleaning up the dsa during resowner release or process exit. What am I missing here? Greetings, Andres Freund ^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: pgsql: Add function to get memory context stats for processes @ 2025-04-22 16:58 Andres Freund <[email protected]> parent: Andres Freund <[email protected]> 1 sibling, 0 replies; 13+ messages in thread From: Andres Freund @ 2025-04-22 16:58 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: Daniel Gustafsson <[email protected]>; PostgreSQL Hackers <[email protected]>; Rahila Syed <[email protected]> Hi, On 2025-04-17 10:42:45 -0400, Robert Haas wrote: > On Tue, Apr 15, 2025 at 6:11 AM Andres Freund <[email protected]> wrote: > > There very well could be a CFI - but it better be somewhere where the > > in-memory state is consistent. Otherwise an error inside raised in the CFI > > would lead the in-memory state inconsistent which then would cause problems > > when cleaning up the dsa during resowner release or process exit. > > > > What am I missing here? > > I think maybe you're only thinking about gathering the data. What > about publishing it? If the DSA code were interrupted at a CFI and the > interrupting code went and tried to perform a DSA allocation to store > the resulting data and then returned to the interrupted DSA operation, > would you expect the code to cope with that? I would expect the DSA code to not call CFI() in such a place - afaict nearly all such cases would also not be safe against errors being raised in the CFI() and then later again allocating memory from the DSA (or resetting it). Similarly, aset.c better not accept interrupts in the middle of, e.g., AllocSetAllocFromNewBlock(), since we'd loose track of the allocation. Greetings, Andres Freund ^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: pgsql: Add function to get memory context stats for processes @ 2025-04-23 18:14 Tom Lane <[email protected]> parent: Andres Freund <[email protected]> 1 sibling, 1 reply; 13+ messages in thread From: Tom Lane @ 2025-04-23 18:14 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: Daniel Gustafsson <[email protected]>; Andres Freund <[email protected]>; PostgreSQL Hackers <[email protected]>; Rahila Syed <[email protected]> Robert Haas <[email protected]> writes: > My primary concern about the patch is that > ProcessGetMemoryContextInterrupt() can be called from any > CHECK_FOR_INTERRUPTS() and calls lots of DSA functions, including > dsa_create() and, via PublishMemoryContext(), dsa_allocate0(). I'm > shocked to hear that you and Andres think that's safe to do at any > current or future CHECK_FOR_INTERRUPTS() anywhere in the code; but > Andres seems very confident that it's fine, so perhaps I should just > stop worrying and be happy that we have the feature. Just for the record, it sounds quite unsafe to me too. I could credit it being all right to examine the process' MemoryContext data structures, but calling dsa_create() from CFI seems really insane. Way too many moving parts there. regards, tom lane ^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: pgsql: Add function to get memory context stats for processes @ 2025-04-26 21:07 Tomas Vondra <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 1 reply; 13+ messages in thread From: Tomas Vondra @ 2025-04-26 21:07 UTC (permalink / raw) To: Tom Lane <[email protected]>; Robert Haas <[email protected]>; +Cc: Daniel Gustafsson <[email protected]>; Andres Freund <[email protected]>; PostgreSQL Hackers <[email protected]>; Rahila Syed <[email protected]> On 4/23/25 20:14, Tom Lane wrote: > Robert Haas <[email protected]> writes: >> My primary concern about the patch is that >> ProcessGetMemoryContextInterrupt() can be called from any >> CHECK_FOR_INTERRUPTS() and calls lots of DSA functions, including >> dsa_create() and, via PublishMemoryContext(), dsa_allocate0(). I'm >> shocked to hear that you and Andres think that's safe to do at any >> current or future CHECK_FOR_INTERRUPTS() anywhere in the code; but >> Andres seems very confident that it's fine, so perhaps I should just >> stop worrying and be happy that we have the feature. > > Just for the record, it sounds quite unsafe to me too. I could > credit it being all right to examine the process' MemoryContext data > structures, but calling dsa_create() from CFI seems really insane. > Way too many moving parts there. > Maybe I'm oblivious to some very obvious issues, but why is this so different from everything else that is already called from ProcessInterrupts()? Perhaps dsa_create() is more complex compared to the other stuff (haven't checked), but why would it be unsafe? The one risk I can think of is a risk of recursion - if any of the functions called from ProcessGetMemoryContextInterrupt() does CFI, could that be a problem if there's another pending signal. I see some other handlers (e.g. ProcessParallelApplyMessages) handle this by explicitly holding interrupts. Should ProcessGetMemoryContextInterrupt() do the same thing? In any case, if DSA happens to not be the right way to transfer this, what should we use instead? The only thing I can think of is some sort of pre-allocated chunk of shared memory. regards -- Tomas Vondra ^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: pgsql: Add function to get memory context stats for processes @ 2025-04-30 20:17 Robert Haas <[email protected]> parent: Tomas Vondra <[email protected]> 0 siblings, 1 reply; 13+ messages in thread From: Robert Haas @ 2025-04-30 20:17 UTC (permalink / raw) To: Tomas Vondra <[email protected]>; +Cc: Tom Lane <[email protected]>; Daniel Gustafsson <[email protected]>; Andres Freund <[email protected]>; PostgreSQL Hackers <[email protected]>; Rahila Syed <[email protected]> On Sat, Apr 26, 2025 at 5:07 PM Tomas Vondra <[email protected]> wrote: > > Just for the record, it sounds quite unsafe to me too. I could > > credit it being all right to examine the process' MemoryContext data > > structures, but calling dsa_create() from CFI seems really insane. > > Way too many moving parts there. > > Maybe I'm oblivious to some very obvious issues, but why is this so > different from everything else that is already called from > ProcessInterrupts()? Perhaps dsa_create() is more complex compared to > the other stuff (haven't checked), but why would it be unsafe? > > The one risk I can think of is a risk of recursion - if any of the > functions called from ProcessGetMemoryContextInterrupt() does CFI, could > that be a problem if there's another pending signal. I see some other > handlers (e.g. ProcessParallelApplyMessages) handle this by explicitly > holding interrupts. Should ProcessGetMemoryContextInterrupt() do the > same thing? > > In any case, if DSA happens to not be the right way to transfer this, > what should we use instead? The only thing I can think of is some sort > of pre-allocated chunk of shared memory. The big disadvantage of a pre-allocated chunk of shared memory is that it would necessarily be fixed size, and a memory context dump can be really big. Another alternative would be a temporary file. But none of that settles the question of safety. I think part of the reason why it's possible for us to have disagreements about whether this is safe is that we don't have any clear documentation of what you can assume to be true at a CHECK_FOR_INTERRUPTS(). It's not possible for there to be an LWLock held at that point, because we hold off interrupts when you acquire an LWLock and don't re-enable them until all LWLocks have been released. We can't be holding a spinlock, either, because we only insert CHECK_FOR_INTERRUPTS() at the top of loops and code that holds a spinlock is supposed to be straight-line code that never loops. But what else can you assume? Can you assume, for example, that there's a transaction? I doubt it. Can you assume that the transaction is non-aborted? I doubt that even more. There's no obvious-to-me reason why those things should be true. And in fact if you try this on a backend waiting in an aborted transaction, it breaks: robert.haas=# select pg_backend_pid(); pg_backend_pid ---------------- 19321 (1 row) robert.haas=# begin; BEGIN robert.haas=*# select 1/0; ERROR: division by zero And then from another session, run this command, using the PID from above: select * from pg_get_process_memory_contexts(19321, false, 1); Then you get: 2025-04-30 15:14:33.878 EDT [19321] ERROR: ResourceOwnerEnlarge called after release started 2025-04-30 15:14:33.878 EDT [19321] FATAL: terminating connection because protocol synchronization was lost I kind of doubt whether that's the only problem here, but it's *a* problem. -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: pgsql: Add function to get memory context stats for processes @ 2025-04-30 20:29 Daniel Gustafsson <[email protected]> parent: Robert Haas <[email protected]> 0 siblings, 1 reply; 13+ messages in thread From: Daniel Gustafsson @ 2025-04-30 20:29 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: Tomas Vondra <[email protected]>; Tom Lane <[email protected]>; Andres Freund <[email protected]>; PostgreSQL Hackers <[email protected]>; Rahila Syed <[email protected]> > On 30 Apr 2025, at 22:17, Robert Haas <[email protected]> wrote: > I kind of doubt whether that's the only problem here, but it's *a* problem. This is indeed exposing a pre-existing issue, which was reported in [0] and a patch fixing it has been submitted. I have been testing and reworking the patch slightly but due to $life and $work events have yet to have time to push it. -- Daniel Gustafsson [0] [email protected] ^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: pgsql: Add function to get memory context stats for processes @ 2025-04-30 22:00 Robert Haas <[email protected]> parent: Daniel Gustafsson <[email protected]> 0 siblings, 0 replies; 13+ messages in thread From: Robert Haas @ 2025-04-30 22:00 UTC (permalink / raw) To: Daniel Gustafsson <[email protected]>; +Cc: Tomas Vondra <[email protected]>; Tom Lane <[email protected]>; Andres Freund <[email protected]>; PostgreSQL Hackers <[email protected]>; Rahila Syed <[email protected]> On Wed, Apr 30, 2025 at 4:29 PM Daniel Gustafsson <[email protected]> wrote: > This is indeed exposing a pre-existing issue, which was reported in [0] and a > patch fixing it has been submitted. I have been testing and reworking the > patch slightly but due to $life and $work events have yet to have time to push > it. Thanks for the pointer. I will reply to that thread. -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 13+ messages in thread
end of thread, other threads:[~2025-04-30 22:00 UTC | newest] Thread overview: 13+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-02-23 00:45 [PATCH 4/7] Add explain(MACHINE) to hide machine-dependent output.. Justin Pryzby <[email protected]> 2025-04-08 09:10 pgsql: Add function to get memory context stats for processes Daniel Gustafsson <[email protected]> 2025-04-10 11:42 ` Re: pgsql: Add function to get memory context stats for processes Laurenz Albe <[email protected]> 2025-04-10 11:48 ` Re: pgsql: Add function to get memory context stats for processes Aleksander Alekseev <[email protected]> 2025-04-10 12:46 ` Re: pgsql: Add function to get memory context stats for processes Daniel Gustafsson <[email protected]> 2025-04-10 20:05 ` Re: pgsql: Add function to get memory context stats for processes Andres Freund <[email protected]> 2025-04-14 14:13 ` Re: pgsql: Add function to get memory context stats for processes Andres Freund <[email protected]> 2025-04-22 16:58 ` Re: pgsql: Add function to get memory context stats for processes Andres Freund <[email protected]> 2025-04-23 18:14 ` Re: pgsql: Add function to get memory context stats for processes Tom Lane <[email protected]> 2025-04-26 21:07 ` Re: pgsql: Add function to get memory context stats for processes Tomas Vondra <[email protected]> 2025-04-30 20:17 ` Re: pgsql: Add function to get memory context stats for processes Robert Haas <[email protected]> 2025-04-30 20:29 ` Re: pgsql: Add function to get memory context stats for processes Daniel Gustafsson <[email protected]> 2025-04-30 22:00 ` Re: pgsql: Add function to get memory context stats for processes Robert Haas <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox