Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1kehel-0008Bh-NB for pgsql-hackers@arkaria.postgresql.org; Mon, 16 Nov 2020 16:47:07 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1kehek-0001EF-E7 for pgsql-hackers@arkaria.postgresql.org; Mon, 16 Nov 2020 16:47:06 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1kehek-0001E7-6A for pgsql-hackers@lists.postgresql.org; Mon, 16 Nov 2020 16:47:06 +0000 Received: from oss.nttdata.com ([49.212.34.109]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1kehec-0000H5-2i for pgsql-hackers@lists.postgresql.org; Mon, 16 Nov 2020 16:47:05 +0000 Received: from hnk.local (p1295030-ipngn4902funabasi.chiba.ocn.ne.jp [153.166.85.30]) by oss.nttdata.com (Postfix) with ESMTPSA id CEAB86151A; Tue, 17 Nov 2020 01:46:52 +0900 (JST) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.102.3 at oss.nttdata.com Subject: Re: [PATCH] Add features to pg_stat_statements To: Seino Yuki Cc: pgsql-hackers@lists.postgresql.org References: <0d9f1107772cf5c3f954e985464c7298@oss.nttdata.com> <160250511275.1171.7692218521107860164.pgcf@coridan.postgresql.org> <369018a4-746c-6914-4983-1b79aa9fdd37@oss.nttdata.com> <4ab1bf5ab895d44ff0a3b3d06d1f85b2@oss.nttdata.com> <5d13be75-ed32-c500-f029-5838ba77d6c9@oss.nttdata.com> <2f749a3bd1b49f1d282abf59619fe1f5@oss.nttdata.com> <050fd52dcddac965ad24a636c25a5611@oss.nttdata.com> <2e9d596e-5b33-3368-682b-87431b65ce8f@oss.nttdata.com> From: Fujii Masao Message-ID: Date: Tue, 17 Nov 2020 01:46:52 +0900 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:78.0) Gecko/20100101 Thunderbird/78.4.3 MIME-Version: 1.0 In-Reply-To: Content-Type: multipart/mixed; boundary="------------B5912D3159F6D1E3507E4AD7" Content-Language: en-US List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk This is a multi-part message in MIME format. --------------B5912D3159F6D1E3507E4AD7 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 8bit On 2020/11/16 12:22, Seino Yuki wrote: >> Thanks for updating the patch! >> >> +        pgss_info->dealloc = 0; >> +        SpinLockInit(&pgss_info->mutex); >> +        Assert(pgss_info->dealloc == 0); >> >> Why is this assertion check necessary? It seems not necessary. >> >> +    { >> +        Assert(found == found_info); >> >> Having pgssSharedState and pgssInfoCounters separately might make >> the code a bit more complicated like the above? If this is true, what about >> including pgssInfoCounters in pgssSharedState? >> >> PGSS_FILE_HEADER needs to be changed since the patch changes >> the format of pgss file? >> >> +    /* Read pgss_info */ >> +    if (feof(file) == 0) >> +        if (fread(pgss_info, sizeof(pgssInfoCounters), 1, file) != 1) >> +            goto read_error; >> >> Why does feof(file) need to be called here? >> >> +pgss_info_update(void) >> +{ >> +    { >> >> Why is the second "{" necessary? It seems redundant. >> >> +pgss_info_reset(void) >> +{ >> +    { >> >> Same as above. >> >> +pg_stat_statements_info(PG_FUNCTION_ARGS) >> +{ >> +    int64        d_count = 0; >> +    { >> >> Same as above. >> >> +        SpinLockAcquire(&c->mutex); >> +        d_count = Int64GetDatum(c->dealloc); >> +        SpinLockRelease(&c->mutex); >> >> Why does Int64GetDatum() need to be called here? It seems not necessary. >> >> +   >> +    >> +     pg_stat_statements_info() returns bigint >> +     >> +      pg_stat_statements_info >> +     >> +    >> >> Isn't it better not to expose pg_stat_statements_info() function in the >> document because pg_stat_statements_info view is enough and there >> seems no use case for the function? >> >> Regards, > > Thanks for the comment. > I'll post a fixed patch. > Due to similar fixed, we have also merged the patches discussed in the following thread. > https://commitfest.postgresql.org/30/2738/ I agree that these two patches should use the same infrastructure because they both try to add the global stats for pg_stat_statements. But IMO they should not be merged to one patch. It's better to develop them one by one for ease of review. Thought? So I extracted the "dealloc" part from the merged version of your patch. Also I refactored the code and applied some cosmetic changes into the patch. Attached is the updated version of the patch that implements only "dealloc" part. Could you review this version? Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION --------------B5912D3159F6D1E3507E4AD7 Content-Type: text/plain; charset=UTF-8; x-mac-type="0"; x-mac-creator="0"; name="pg_stat_statements_info_v3.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="pg_stat_statements_info_v3.patch" diff --git a/contrib/pg_stat_statements/Makefile b/contrib/pg_stat_statements/Makefile index 081f997d70..3ec627b956 100644 --- a/contrib/pg_stat_statements/Makefile +++ b/contrib/pg_stat_statements/Makefile @@ -6,7 +6,7 @@ OBJS = \ pg_stat_statements.o EXTENSION = pg_stat_statements -DATA = pg_stat_statements--1.4.sql \ +DATA = pg_stat_statements--1.4.sql pg_stat_statements--1.8--1.9.sql \ pg_stat_statements--1.7--1.8.sql pg_stat_statements--1.6--1.7.sql \ pg_stat_statements--1.5--1.6.sql pg_stat_statements--1.4--1.5.sql \ pg_stat_statements--1.3--1.4.sql pg_stat_statements--1.2--1.3.sql \ diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out b/contrib/pg_stat_statements/expected/pg_stat_statements.out index 2a303a7f07..16158525ca 100644 --- a/contrib/pg_stat_statements/expected/pg_stat_statements.out +++ b/contrib/pg_stat_statements/expected/pg_stat_statements.out @@ -861,4 +861,19 @@ SELECT query, plans, calls, rows FROM pg_stat_statements ORDER BY query COLLATE SELECT query, plans, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C" | 1 | 0 | 0 (6 rows) +-- +-- access to pg_stat_statements_info view +-- +SELECT pg_stat_statements_reset(); + pg_stat_statements_reset +-------------------------- + +(1 row) + +SELECT dealloc FROM pg_stat_statements_info; + dealloc +--------- + 0 +(1 row) + DROP EXTENSION pg_stat_statements; diff --git a/contrib/pg_stat_statements/pg_stat_statements--1.8--1.9.sql b/contrib/pg_stat_statements/pg_stat_statements--1.8--1.9.sql new file mode 100644 index 0000000000..2019a4ffe0 --- /dev/null +++ b/contrib/pg_stat_statements/pg_stat_statements--1.8--1.9.sql @@ -0,0 +1,17 @@ +/* contrib/pg_stat_statements/pg_stat_statements--1.8--1.9.sql */ + +-- complain if script is sourced in psql, rather than via ALTER EXTENSION +\echo Use "ALTER EXTENSION pg_stat_statements UPDATE TO '1.9'" to load this file. \quit + +--- Define pg_stat_statements_info +CREATE FUNCTION pg_stat_statements_info( + OUT dealloc bigint +) +RETURNS bigint +AS 'MODULE_PATHNAME' +LANGUAGE C STRICT VOLATILE PARALLEL SAFE; + +CREATE VIEW pg_stat_statements_info AS + SELECT * FROM pg_stat_statements_info(); + +GRANT SELECT ON pg_stat_statements_info TO PUBLIC; diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c index dd963c4644..9de2d53496 100644 --- a/contrib/pg_stat_statements/pg_stat_statements.c +++ b/contrib/pg_stat_statements/pg_stat_statements.c @@ -98,7 +98,7 @@ PG_MODULE_MAGIC; #define PGSS_TEXT_FILE PG_STAT_TMP_DIR "/pgss_query_texts.stat" /* Magic number identifying the stats file format */ -static const uint32 PGSS_FILE_HEADER = 0x20171004; +static const uint32 PGSS_FILE_HEADER = 0x20201116; /* PostgreSQL major version number, changes in which invalidate all entries */ static const uint32 PGSS_PG_MAJOR_VERSION = PG_VERSION_NUM / 100; @@ -193,6 +193,14 @@ typedef struct Counters uint64 wal_bytes; /* total amount of WAL bytes generated */ } Counters; +/* + * Global statistics for pg_stat_statements + */ +typedef struct pgssGlobalStats +{ + int64 dealloc; /* # of times entries were deallocated */ +} pgssGlobalStats; + /* * Statistics per statement * @@ -222,6 +230,7 @@ typedef struct pgssSharedState Size extent; /* current extent of query file */ int n_writers; /* number of active writers to query file */ int gc_count; /* query file garbage collection cycle count */ + pgssGlobalStats stats; /* global statistics for pgss */ } pgssSharedState; /* @@ -327,6 +336,7 @@ PG_FUNCTION_INFO_V1(pg_stat_statements_1_2); PG_FUNCTION_INFO_V1(pg_stat_statements_1_3); PG_FUNCTION_INFO_V1(pg_stat_statements_1_8); PG_FUNCTION_INFO_V1(pg_stat_statements); +PG_FUNCTION_INFO_V1(pg_stat_statements_info); static void pgss_shmem_startup(void); static void pgss_shmem_shutdown(int code, Datum arg); @@ -554,6 +564,7 @@ pgss_shmem_startup(void) pgss->extent = 0; pgss->n_writers = 0; pgss->gc_count = 0; + pgss->stats.dealloc = 0; } memset(&info, 0, sizeof(info)); @@ -673,6 +684,10 @@ pgss_shmem_startup(void) entry->counters = temp.counters; } + /* Read global statistics for pg_stat_statements */ + if (fread(&pgss->stats, sizeof(pgssGlobalStats), 1, file) != 1) + goto read_error; + pfree(buffer); FreeFile(file); FreeFile(qfile); @@ -794,6 +809,10 @@ pgss_shmem_shutdown(int code, Datum arg) } } + /* Dump global statistics for pg_stat_statements */ + if (fwrite(&pgss->stats, sizeof(pgssGlobalStats), 1, file) != 1) + goto error; + free(qbuffer); qbuffer = NULL; @@ -1863,6 +1882,26 @@ pg_stat_statements_internal(FunctionCallInfo fcinfo, tuplestore_donestoring(tupstore); } +/* + * Return statistics of pg_stat_statements. + */ +Datum +pg_stat_statements_info(PG_FUNCTION_ARGS) +{ + pgssGlobalStats stats; + + /* Read global statistics for pg_stat_statements */ + { + volatile pgssSharedState *s = (volatile pgssSharedState *) pgss; + + SpinLockAcquire(&s->mutex); + stats = s->stats; + SpinLockRelease(&s->mutex); + } + + PG_RETURN_INT64(stats.dealloc); +} + /* * Estimate shared memory space needed. */ @@ -2018,6 +2057,15 @@ entry_dealloc(void) } pfree(entries); + + /* Increment the number of times entries are deallocated */ + { + volatile pgssSharedState *s = (volatile pgssSharedState *) pgss; + + SpinLockAcquire(&s->mutex); + s->stats.dealloc += 1; + SpinLockRelease(&s->mutex); + } } /* @@ -2504,6 +2552,15 @@ entry_reset(Oid userid, Oid dbid, uint64 queryid) hash_search(pgss_hash, &entry->key, HASH_REMOVE, NULL); num_remove++; } + + /* Reset global statistics for pg_stat_statements */ + { + volatile pgssSharedState *s = (volatile pgssSharedState *) pgss; + + SpinLockAcquire(&s->mutex); + s->stats.dealloc = 0; + SpinLockRelease(&s->mutex); + } } /* All entries are removed? */ diff --git a/contrib/pg_stat_statements/pg_stat_statements.control b/contrib/pg_stat_statements/pg_stat_statements.control index 65b18b11d2..2f1ce6ed50 100644 --- a/contrib/pg_stat_statements/pg_stat_statements.control +++ b/contrib/pg_stat_statements/pg_stat_statements.control @@ -1,5 +1,5 @@ # pg_stat_statements extension comment = 'track planning and execution statistics of all SQL statements executed' -default_version = '1.8' +default_version = '1.9' module_pathname = '$libdir/pg_stat_statements' relocatable = true diff --git a/contrib/pg_stat_statements/sql/pg_stat_statements.sql b/contrib/pg_stat_statements/sql/pg_stat_statements.sql index e9f5bb84e3..6f58d9d0f6 100644 --- a/contrib/pg_stat_statements/sql/pg_stat_statements.sql +++ b/contrib/pg_stat_statements/sql/pg_stat_statements.sql @@ -358,4 +358,10 @@ SELECT 42; SELECT 42; SELECT query, plans, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C"; +-- +-- access to pg_stat_statements_info view +-- +SELECT pg_stat_statements_reset(); +SELECT dealloc FROM pg_stat_statements_info; + DROP EXTENSION pg_stat_statements; diff --git a/doc/src/sgml/pgstatstatements.sgml b/doc/src/sgml/pgstatstatements.sgml index cf2d25b7b2..b0c8d6f0f1 100644 --- a/doc/src/sgml/pgstatstatements.sgml +++ b/doc/src/sgml/pgstatstatements.sgml @@ -23,7 +23,9 @@ When pg_stat_statements is loaded, it tracks statistics across all databases of the server. To access and manipulate - these statistics, the module provides a view, pg_stat_statements, + these statistics, the module provides views + pg_stat_statements and + pg_stat_statements_info, and the utility functions pg_stat_statements_reset and pg_stat_statements. These are not available globally but can be enabled for a specific database with @@ -480,6 +482,50 @@ + + The <structname>pg_stat_statements_info</structname> View + + + pg_stat_statements_info + + + + The statistics of the pg_stat_statements module + itself are tracked and made available via a view named + pg_stat_statements_info. This view contains + only a single row. The columns of the view are shown in + . + + + + <structname>pg_stat_statements_info</structname> Columns + + + + + Column Type + + + Description + + + + + + + + dealloc bigint + + + Total number of times pg_stat_statements + entries were deallocated + + + + +
+ + Functions @@ -501,9 +547,10 @@ specified, the default value 0(invalid) is used for each of them and the statistics that match with other parameters will be reset. If no parameter is specified or all the specified parameters are - 0(invalid), it will discard all statistics. By - default, this function can only be executed by superusers. Access may be - granted to others using GRANT. + 0(invalid), it will discard all statistics including + the statistics that pg_stat_statements_info + displays. By default, this function can only be executed by superusers. + Access may be granted to others using GRANT. diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list index b146b3ea73..65f59ea09f 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -3215,6 +3215,7 @@ pgpid_t pgsocket pgsql_thing_t pgssEntry +pgssGlobalStats pgssHashKey pgssJumbleState pgssLocationLen --------------B5912D3159F6D1E3507E4AD7--