public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v10 5/7] Row pattern recognition patch (docs). 7+ messages / 4 participants [nested] [flat]
* [PATCH v10 5/7] Row pattern recognition patch (docs). @ 2023-10-22 02:22 Tatsuo Ishii <[email protected]> 0 siblings, 0 replies; 7+ messages in thread From: Tatsuo Ishii @ 2023-10-22 02:22 UTC (permalink / raw) --- doc/src/sgml/advanced.sgml | 80 ++++++++++++++++++++++++++++++++++++ doc/src/sgml/func.sgml | 54 ++++++++++++++++++++++++ doc/src/sgml/ref/select.sgml | 38 ++++++++++++++++- 3 files changed, 170 insertions(+), 2 deletions(-) diff --git a/doc/src/sgml/advanced.sgml b/doc/src/sgml/advanced.sgml index 755c9f1485..cf18dd887e 100644 --- a/doc/src/sgml/advanced.sgml +++ b/doc/src/sgml/advanced.sgml @@ -537,6 +537,86 @@ WHERE pos < 3; <literal>rank</literal> less than 3. </para> + <para> + Row pattern common syntax can be used to perform row pattern recognition + in a query. Row pattern common syntax includes two sub + clauses: <literal>DEFINE</literal> + and <literal>PATTERN</literal>. <literal>DEFINE</literal> defines + definition variables along with an expression. The expression must be a + logical expression, which means it must + return <literal>TRUE</literal>, <literal>FALSE</literal> + or <literal>NULL</literal>. The expression may comprise column references + and functions. Window functions, aggregate functions and subqueries are + not allowed. An example of <literal>DEFINE</literal> is as follows. + +<programlisting> +DEFINE + LOWPRICE AS price <= 100, + UP AS price > PREV(price), + DOWN AS price < PREV(price) +</programlisting> + + Note that <function>PREV</function> returns the price column in the + previous row if it's called in a context of row pattern recognition. So in + the second line the definition variable "UP" is <literal>TRUE</literal> + when the price column in the current row is greater than the price column + in the previous row. Likewise, "DOWN" is <literal>TRUE</literal> when when + the price column in the current row is lower than the price column in the + previous row. + </para> + <para> + Once <literal>DEFINE</literal> exists, <literal>PATTERN</literal> can be + used. <literal>PATTERN</literal> defines a sequence of rows that satisfies + certain conditions. For example following <literal>PATTERN</literal> + defines that a row starts with the condition "LOWPRICE", then one or more + rows satisfy "UP" and finally one or more rows satisfy "DOWN". Note that + "+" means one or more matches. Also you can use "*", which means zero or + more matches. If a sequence of rows which satisfies the PATTERN is found, + in the starting row of the sequence of rows all window functions and + aggregates are shown in the target list. Note that aggregations only look + into the matched rows, rather than whole frame. In the second or + subsequent rows all window functions and aggregates are NULL. For rows + that do not match the PATTERN, all window functions and aggregates are + shown AS NULL too, except count which shows 0. This is because the + unmatched rows are in an empty frame. Example of + a <literal>SELECT</literal> using the <literal>DEFINE</literal> + and <literal>PATTERN</literal> clause is as follows. + +<programlisting> +SELECT company, tdate, price, + first_value(price) OVER w, + max(price) OVER w, + count(price) OVER w +FROM stock, + WINDOW w AS ( + PARTITION BY company + ORDER BY tdate + ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING + AFTER MATCH SKIP PAST LAST ROW + INITIAL + PATTERN (LOWPRICE UP+ DOWN+) + DEFINE + LOWPRICE AS price <= 100, + UP AS price > PREV(price), + DOWN AS price < PREV(price) +); +</programlisting> +<screen> + company | tdate | price | first_value | max | count +----------+------------+-------+-------------+-----+------- + company1 | 2023-07-01 | 100 | 100 | 200 | 4 + company1 | 2023-07-02 | 200 | | | + company1 | 2023-07-03 | 150 | | | + company1 | 2023-07-04 | 140 | | | + company1 | 2023-07-05 | 150 | | | 0 + company1 | 2023-07-06 | 90 | 90 | 130 | 4 + company1 | 2023-07-07 | 110 | | | + company1 | 2023-07-08 | 130 | | | + company1 | 2023-07-09 | 120 | | | + company1 | 2023-07-10 | 130 | | | 0 +</screen> + </para> + <para> When a query involves multiple window functions, it is possible to write out each one with a separate <literal>OVER</literal> clause, but this is diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml index 7c3e940afe..1d835af15a 100644 --- a/doc/src/sgml/func.sgml +++ b/doc/src/sgml/func.sgml @@ -21878,6 +21878,7 @@ SELECT count(*) FROM sometable; returns <literal>NULL</literal> if there is no such row. </para></entry> </row> + </tbody> </tgroup> </table> @@ -21917,6 +21918,59 @@ SELECT count(*) FROM sometable; Other frame specifications can be used to obtain other effects. </para> + <para> + Row pattern recognition navigation functions are listed in + <xref linkend="functions-rpr-navigation-table"/>. These functions + can be used to describe DEFINE clause of Row pattern recognition. + </para> + + <table id="functions-rpr-navigation-table"> + <title>Row Pattern Navigation Functions</title> + <tgroup cols="1"> + <thead> + <row> + <entry role="func_table_entry"><para role="func_signature"> + Function + </para> + <para> + Description + </para></entry> + </row> + </thead> + + <tbody> + <row> + <entry role="func_table_entry"><para role="func_signature"> + <indexterm> + <primary>prev</primary> + </indexterm> + <function>prev</function> ( <parameter>value</parameter> <type>anyelement</type> ) + <returnvalue>anyelement</returnvalue> + </para> + <para> + Returns the column value at the previous row; + returns NULL if there is no previous row in the window frame. + </para></entry> + </row> + + <row> + <entry role="func_table_entry"><para role="func_signature"> + <indexterm> + <primary>next</primary> + </indexterm> + <function>next</function> ( <parameter>value</parameter> <type>anyelement</type> ) + <returnvalue>anyelement</returnvalue> + </para> + <para> + Returns the column value at the next row; + returns NULL if there is no next row in the window frame. + </para></entry> + </row> + + </tbody> + </tgroup> + </table> + <note> <para> The SQL standard defines a <literal>RESPECT NULLS</literal> or diff --git a/doc/src/sgml/ref/select.sgml b/doc/src/sgml/ref/select.sgml index 42d78913cf..522ad9dd70 100644 --- a/doc/src/sgml/ref/select.sgml +++ b/doc/src/sgml/ref/select.sgml @@ -969,8 +969,8 @@ WINDOW <replaceable class="parameter">window_name</replaceable> AS ( <replaceabl The <replaceable class="parameter">frame_clause</replaceable> can be one of <synopsis> -{ RANGE | ROWS | GROUPS } <replaceable>frame_start</replaceable> [ <replaceable>frame_exclusion</replaceable> ] -{ RANGE | ROWS | GROUPS } BETWEEN <replaceable>frame_start</replaceable> AND <replaceable>frame_end</replaceable> [ <replaceable>frame_exclusion</replaceable> ] +{ RANGE | ROWS | GROUPS } <replaceable>frame_start</replaceable> [ <replaceable>frame_exclusion</replaceable> ] [row_pattern_common_syntax] +{ RANGE | ROWS | GROUPS } BETWEEN <replaceable>frame_start</replaceable> AND <replaceable>frame_end</replaceable> [ <replaceable>frame_exclusion</replaceable> ] [row_pattern_common_syntax] </synopsis> where <replaceable>frame_start</replaceable> @@ -1077,6 +1077,40 @@ EXCLUDE NO OTHERS a given peer group will be in the frame or excluded from it. </para> + <para> + The + optional <replaceable class="parameter">row_pattern_common_syntax</replaceable> + defines the <firstterm>row pattern recognition condition</firstterm> for + this + window. <replaceable class="parameter">row_pattern_common_syntax</replaceable> + includes following subclauses. <literal>AFTER MATCH SKIP PAST LAST + ROW</literal> or <literal>AFTER MATCH SKIP TO NEXT ROW</literal> controls + how to proceed to next row position after a match + found. With <literal>AFTER MATCH SKIP PAST LAST ROW</literal> (the + default) next row position is next to the last row of previous match. On + the other hand, with <literal>AFTER MATCH SKIP TO NEXT ROW</literal> next + row position is always next to the last row of previous + match. <literal>DEFINE</literal> defines definition variables along with a + boolean expression. <literal>PATTERN</literal> defines a sequence of rows + that satisfies certain conditions using variables defined + in <literal>DEFINE</literal> clause. If the variable is not defined in + the <literal>DEFINE</literal> clause, it is implicitly assumed + following is defined in the <literal>DEFINE</literal> clause. + +<synopsis> +<literal>variable_name</literal> AS TRUE +</synopsis> + + Note that the maximu number of variables defined + in <literal>DEFINE</literal> clause is 26. + +<synopsis> +[ AFTER MATCH SKIP PAST LAST ROW | AFTER MATCH SKIP TO NEXT ROW ] +PATTERN <replaceable class="parameter">pattern_variable_name</replaceable>[+] [, ...] +DEFINE <replaceable class="parameter">definition_varible_name</replaceable> AS <replaceable class="parameter">expression</replaceable> [, ...] +</synopsis> + </para> + <para> The purpose of a <literal>WINDOW</literal> clause is to specify the behavior of <firstterm>window functions</firstterm> appearing in the query's -- 2.25.1 ----Next_Part(Sun_Oct_22_11_39_20_2023_140)-- Content-Type: Text/X-Patch; charset=us-ascii Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="v10-0006-Row-pattern-recognition-patch-tests.patch" ^ permalink raw reply [nested|flat] 7+ messages in thread
* Add callbacks for fixed-numbered stats flush in pgstats @ 2024-09-03 04:48 Michael Paquier <[email protected]> 0 siblings, 1 reply; 7+ messages in thread From: Michael Paquier @ 2024-09-03 04:48 UTC (permalink / raw) To: Postgres hackers <[email protected]> Hi all, The last TODO item I had in my bucket about the generalization of pgstats is the option to a better control on the flush of the stats depending on the kind for fixed-numbered stats. Currently, this is controlled by pgstat_report_stat(), that includes special handling for WAL, IO and SLRU stats, with two generic concepts: - Check if there are pending entries, allowing a fast-path exit. - Do the actual flush, with a recheck on pending entries. SLRU and IO control that with one variable each, and WAL uses a routine for the same called pgstat_have_pending_wal(). Please find attached a patch to generalize the concept, with two new callbacks that can be used for fixed-numbered stats. SLRU, IO and WAL are switched to use these (the two pgstat_flush_* routines have been kept on purpose). This brings some clarity in the code, by making have_iostats and have_slrustats static in their respective files. The two pgstat_flush_* wrappers do not need a boolean as return result. Running Postgres on scissors with a read-only workload that does not trigger stats, I was not able to see a difference in runtime, but that was on my own laptop, and I am planning to do more measurements on a bigger machine. This is in the same line of thoughts as the recent thread about the backend init callback, generalizing more the whole facility: https://www.postgresql.org/message-id/[email protected] Like the other one, I wanted to send that a few days ago, but well, life likes going its own ways sometimes. Thanks, -- Michael Attachments: [text/x-diff] 0001-Add-callbacks-to-control-flush-of-fixed-numbered-sta.patch (10.7K, ../../[email protected]/2-0001-Add-callbacks-to-control-flush-of-fixed-numbered-sta.patch) download | inline diff: From e98f8d50c17cd8fc521bffb4bdc73ef58b7fa430 Mon Sep 17 00:00:00 2001 From: Michael Paquier <[email protected]> Date: Tue, 3 Sep 2024 13:36:10 +0900 Subject: [PATCH] Add callbacks to control flush of fixed-numbered stats This commit adds two callbacks in pgstats: - have_fixed_pending_cb, to check if a stats kind has any pending stuff waiting for a flush. - flush_fixed_cb, to do the flush. This is used by the SLRU, WAL and IO statistics, generalizing the concept for all stats kinds (builtin and custom). --- src/include/utils/pgstat_internal.h | 42 +++++++++------- src/backend/utils/activity/pgstat.c | 63 +++++++++++++++++++----- src/backend/utils/activity/pgstat_io.c | 22 ++++++++- src/backend/utils/activity/pgstat_slru.c | 13 ++++- src/backend/utils/activity/pgstat_wal.c | 19 +++++-- 5 files changed, 119 insertions(+), 40 deletions(-) diff --git a/src/include/utils/pgstat_internal.h b/src/include/utils/pgstat_internal.h index fb132e439d..d69aa05b1c 100644 --- a/src/include/utils/pgstat_internal.h +++ b/src/include/utils/pgstat_internal.h @@ -231,7 +231,7 @@ typedef struct PgStat_KindInfo /* * For variable-numbered stats: flush pending stats. Required if pending - * data is used. + * data is used. See flush_fixed_cb for fixed-numbered stats. */ bool (*flush_pending_cb) (PgStat_EntryRef *sr, bool nowait); @@ -259,6 +259,19 @@ typedef struct PgStat_KindInfo */ void (*init_shmem_cb) (void *stats); + /* + * For fixed-numbered statistics: Flush pending stats. Returns true if + * some of the stats could not be flushed, due to lock contention for + * example. Optional. + */ + bool (*flush_fixed_cb) (bool nowait); + + /* + * For fixed-numbered statistics: Check for pending stats in need of + * flush. Returns true if there are any stats pending for flush. Optional. + */ + bool (*have_fixed_pending_cb) (void); + /* * For fixed-numbered statistics: Reset All. */ @@ -603,7 +616,10 @@ extern bool pgstat_function_flush_cb(PgStat_EntryRef *entry_ref, bool nowait); * Functions in pgstat_io.c */ -extern bool pgstat_flush_io(bool nowait); +extern void pgstat_flush_io(bool nowait); + +extern bool pgstat_io_have_pending_cb(void); +extern bool pgstat_io_flush_cb(bool nowait); extern void pgstat_io_init_shmem_cb(void *stats); extern void pgstat_io_reset_all_cb(TimestampTz ts); extern void pgstat_io_snapshot_cb(void); @@ -662,7 +678,8 @@ extern PgStatShared_Common *pgstat_init_entry(PgStat_Kind kind, * Functions in pgstat_slru.c */ -extern bool pgstat_slru_flush(bool nowait); +extern bool pgstat_slru_have_pending_cb(void); +extern bool pgstat_slru_flush_cb(bool nowait); extern void pgstat_slru_init_shmem_cb(void *stats); extern void pgstat_slru_reset_all_cb(TimestampTz ts); extern void pgstat_slru_snapshot_cb(void); @@ -672,10 +689,11 @@ extern void pgstat_slru_snapshot_cb(void); * Functions in pgstat_wal.c */ -extern bool pgstat_flush_wal(bool nowait); extern void pgstat_init_wal(void); -extern bool pgstat_have_pending_wal(void); +extern void pgstat_flush_wal(bool nowait); +extern bool pgstat_wal_have_pending_cb(void); +extern bool pgstat_wal_flush_cb(bool nowait); extern void pgstat_wal_init_shmem_cb(void *stats); extern void pgstat_wal_reset_all_cb(TimestampTz ts); extern void pgstat_wal_snapshot_cb(void); @@ -705,20 +723,6 @@ extern void pgstat_create_transactional(PgStat_Kind kind, Oid dboid, Oid objoid) extern PGDLLIMPORT PgStat_LocalState pgStatLocal; -/* - * Variables in pgstat_io.c - */ - -extern PGDLLIMPORT bool have_iostats; - - -/* - * Variables in pgstat_slru.c - */ - -extern PGDLLIMPORT bool have_slrustats; - - /* * Implementation of inline functions declared above. */ diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c index b2ca3f39b7..8cbf706d5b 100644 --- a/src/backend/utils/activity/pgstat.c +++ b/src/backend/utils/activity/pgstat.c @@ -411,6 +411,8 @@ static const PgStat_KindInfo pgstat_kind_builtin_infos[PGSTAT_KIND_BUILTIN_SIZE] .shared_data_off = offsetof(PgStatShared_IO, stats), .shared_data_len = sizeof(((PgStatShared_IO *) 0)->stats), + .flush_fixed_cb = pgstat_io_flush_cb, + .have_fixed_pending_cb = pgstat_io_have_pending_cb, .init_shmem_cb = pgstat_io_init_shmem_cb, .reset_all_cb = pgstat_io_reset_all_cb, .snapshot_cb = pgstat_io_snapshot_cb, @@ -426,6 +428,8 @@ static const PgStat_KindInfo pgstat_kind_builtin_infos[PGSTAT_KIND_BUILTIN_SIZE] .shared_data_off = offsetof(PgStatShared_SLRU, stats), .shared_data_len = sizeof(((PgStatShared_SLRU *) 0)->stats), + .flush_fixed_cb = pgstat_slru_flush_cb, + .have_fixed_pending_cb = pgstat_slru_have_pending_cb, .init_shmem_cb = pgstat_slru_init_shmem_cb, .reset_all_cb = pgstat_slru_reset_all_cb, .snapshot_cb = pgstat_slru_snapshot_cb, @@ -441,6 +445,8 @@ static const PgStat_KindInfo pgstat_kind_builtin_infos[PGSTAT_KIND_BUILTIN_SIZE] .shared_data_off = offsetof(PgStatShared_Wal, stats), .shared_data_len = sizeof(((PgStatShared_Wal *) 0)->stats), + .flush_fixed_cb = pgstat_wal_flush_cb, + .have_fixed_pending_cb = pgstat_wal_have_pending_cb, .init_shmem_cb = pgstat_wal_init_shmem_cb, .reset_all_cb = pgstat_wal_reset_all_cb, .snapshot_cb = pgstat_wal_snapshot_cb, @@ -661,13 +667,37 @@ pgstat_report_stat(bool force) } /* Don't expend a clock check if nothing to do */ - if (dlist_is_empty(&pgStatPending) && - !have_iostats && - !have_slrustats && - !pgstat_have_pending_wal()) + if (dlist_is_empty(&pgStatPending)) { - Assert(pending_since == 0); - return 0; + bool do_flush = false; + + /* Check for pending fixed-numbered stats */ + for (PgStat_Kind kind = PGSTAT_KIND_MIN; kind <= PGSTAT_KIND_MAX; kind++) + { + const PgStat_KindInfo *kind_info = pgstat_get_kind_info(kind); + + if (!kind_info) + continue; + if (!kind_info->fixed_amount) + { + Assert(kind_info->have_fixed_pending_cb == NULL); + continue; + } + if (!kind_info->have_fixed_pending_cb) + continue; + + if (kind_info->have_fixed_pending_cb()) + { + do_flush = true; + break; + } + } + + if (!do_flush) + { + Assert(pending_since == 0); + return 0; + } } /* @@ -720,14 +750,23 @@ pgstat_report_stat(bool force) /* flush database / relation / function / ... stats */ partial_flush |= pgstat_flush_pending_entries(nowait); - /* flush IO stats */ - partial_flush |= pgstat_flush_io(nowait); + /* flush of fixed-numbered stats */ + for (PgStat_Kind kind = PGSTAT_KIND_MIN; kind <= PGSTAT_KIND_MAX; kind++) + { + const PgStat_KindInfo *kind_info = pgstat_get_kind_info(kind); - /* flush wal stats */ - partial_flush |= pgstat_flush_wal(nowait); + if (!kind_info) + continue; + if (!kind_info->fixed_amount) + { + Assert(kind_info->flush_fixed_cb == NULL); + continue; + } + if (!kind_info->flush_fixed_cb) + continue; - /* flush SLRU stats */ - partial_flush |= pgstat_slru_flush(nowait); + partial_flush |= kind_info->flush_fixed_cb(nowait); + } last_flush = now; diff --git a/src/backend/utils/activity/pgstat_io.c b/src/backend/utils/activity/pgstat_io.c index 8af55989ee..cc2ffc78aa 100644 --- a/src/backend/utils/activity/pgstat_io.c +++ b/src/backend/utils/activity/pgstat_io.c @@ -29,7 +29,7 @@ typedef struct PgStat_PendingIO static PgStat_PendingIO PendingIOStats; -bool have_iostats = false; +static bool have_iostats = false; /* @@ -161,6 +161,24 @@ pgstat_fetch_stat_io(void) return &pgStatLocal.snapshot.io; } +/* + * Check if there any IO stats waiting for flush. + */ +bool +pgstat_io_have_pending_cb(void) +{ + return have_iostats; +} + +/* + * Simpler wrapper of pgstat_io_flush_cb() + */ +void +pgstat_flush_io(bool nowait) +{ + (void) pgstat_io_flush_cb(nowait); +} + /* * Flush out locally pending IO statistics * @@ -170,7 +188,7 @@ pgstat_fetch_stat_io(void) * acquired. Otherwise, return false. */ bool -pgstat_flush_io(bool nowait) +pgstat_io_flush_cb(bool nowait) { LWLock *bktype_lock; PgStat_BktypeIO *bktype_shstats; diff --git a/src/backend/utils/activity/pgstat_slru.c b/src/backend/utils/activity/pgstat_slru.c index 6f922a85bf..dd6f9f840e 100644 --- a/src/backend/utils/activity/pgstat_slru.c +++ b/src/backend/utils/activity/pgstat_slru.c @@ -32,7 +32,7 @@ static void pgstat_reset_slru_counter_internal(int index, TimestampTz ts); * in order to avoid memory allocation. */ static PgStat_SLRUStats pending_SLRUStats[SLRU_NUM_ELEMENTS]; -bool have_slrustats = false; +static bool have_slrustats = false; /* @@ -143,6 +143,15 @@ pgstat_get_slru_index(const char *name) return (SLRU_NUM_ELEMENTS - 1); } +/* + * Check if there are any SLRU stats entries waiting for flush. + */ +bool +pgstat_slru_have_pending_cb(void) +{ + return have_slrustats; +} + /* * Flush out locally pending SLRU stats entries * @@ -153,7 +162,7 @@ pgstat_get_slru_index(const char *name) * acquired. Otherwise return false. */ bool -pgstat_slru_flush(bool nowait) +pgstat_slru_flush_cb(bool nowait) { PgStatShared_SLRU *stats_shmem = &pgStatLocal.shmem->slru; int i; diff --git a/src/backend/utils/activity/pgstat_wal.c b/src/backend/utils/activity/pgstat_wal.c index e2a3f6b865..3d76091f71 100644 --- a/src/backend/utils/activity/pgstat_wal.c +++ b/src/backend/utils/activity/pgstat_wal.c @@ -71,6 +71,15 @@ pgstat_fetch_stat_wal(void) return &pgStatLocal.snapshot.wal; } +/* + * Simple wrapper of pgstat_wal_flush_cb() + */ +void +pgstat_flush_wal(bool nowait) +{ + (void) pgstat_wal_flush_cb(nowait); +} + /* * Calculate how much WAL usage counters have increased by subtracting the * previous counters from the current ones. @@ -79,7 +88,7 @@ pgstat_fetch_stat_wal(void) * acquired. Otherwise return false. */ bool -pgstat_flush_wal(bool nowait) +pgstat_wal_flush_cb(bool nowait) { PgStatShared_Wal *stats_shmem = &pgStatLocal.shmem->wal; WalUsage wal_usage_diff = {0}; @@ -92,7 +101,7 @@ pgstat_flush_wal(bool nowait) * This function can be called even if nothing at all has happened. Avoid * taking lock for nothing in that case. */ - if (!pgstat_have_pending_wal()) + if (!pgstat_wal_have_pending_cb()) return false; /* @@ -141,8 +150,8 @@ void pgstat_init_wal(void) { /* - * Initialize prevWalUsage with pgWalUsage so that pgstat_flush_wal() can - * calculate how much pgWalUsage counters are increased by subtracting + * Initialize prevWalUsage with pgWalUsage so that pgstat_wal_flush_cb() + * can calculate how much pgWalUsage counters are increased by subtracting * prevWalUsage from pgWalUsage. */ prevWalUsage = pgWalUsage; @@ -156,7 +165,7 @@ pgstat_init_wal(void) * data pages. */ bool -pgstat_have_pending_wal(void) +pgstat_wal_have_pending_cb(void) { return pgWalUsage.wal_records != prevWalUsage.wal_records || PendingWalStats.wal_write != 0 || -- 2.45.2 [application/pgp-signature] signature.asc (833B, ../../[email protected]/3-signature.asc) download ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Add callbacks for fixed-numbered stats flush in pgstats @ 2024-09-04 05:05 Kyotaro Horiguchi <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 1 reply; 7+ messages in thread From: Kyotaro Horiguchi @ 2024-09-04 05:05 UTC (permalink / raw) To: [email protected]; +Cc: [email protected] At Tue, 3 Sep 2024 13:48:59 +0900, Michael Paquier <[email protected]> wrote in > Hi all, > > The last TODO item I had in my bucket about the generalization of > pgstats is the option to a better control on the flush of the stats > depending on the kind for fixed-numbered stats. Currently, this is > controlled by pgstat_report_stat(), that includes special handling for > WAL, IO and SLRU stats, with two generic concepts: > - Check if there are pending entries, allowing a fast-path exit. > - Do the actual flush, with a recheck on pending entries. > > SLRU and IO control that with one variable each, and WAL uses a > routine for the same called pgstat_have_pending_wal(). Please find > attached a patch to generalize the concept, with two new callbacks > that can be used for fixed-numbered stats. SLRU, IO and WAL are > switched to use these (the two pgstat_flush_* routines have been kept > on purpose). This brings some clarity in the code, by making > have_iostats and have_slrustats static in their respective files. The > two pgstat_flush_* wrappers do not need a boolean as return result. The generalization sounds good to me, and hiding the private flags in private places also seems good. Regarding pgstat_io_flush_cb, I think it no longer needs to check the have_iostats variable, and that check should be moved to the new pgstat_flush_io(). The same applies to pgstat_wal_flush_cb() (and pgstat_flush_wal()). As for pgstat_slru_flush_cb, it simply doesn't need the check anymore. > Running Postgres on scissors with a read-only workload that does not > trigger stats, I was not able to see a difference in runtime, but that > was on my own laptop, and I am planning to do more measurements on a > bigger machine. I don't think it matters, since the actual flushing occurs at 10-second intervals during busy times. We could change the check from a callback to a variable, but that would just shift the function call overhead to a more frequently called side. > This is in the same line of thoughts as the recent thread about the > backend init callback, generalizing more the whole facility: > https://www.postgresql.org/message-id/[email protected] > > Like the other one, I wanted to send that a few days ago, but well, > life likes going its own ways sometimes. reards. -- Kyotaro Horiguchi NTT Open Source Software Center ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Add callbacks for fixed-numbered stats flush in pgstats @ 2024-09-04 05:28 Bertrand Drouvot <[email protected]> parent: Kyotaro Horiguchi <[email protected]> 0 siblings, 1 reply; 7+ messages in thread From: Bertrand Drouvot @ 2024-09-04 05:28 UTC (permalink / raw) To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected]; [email protected] Hi, On Wed, Sep 04, 2024 at 02:05:46PM +0900, Kyotaro Horiguchi wrote: > At Tue, 3 Sep 2024 13:48:59 +0900, Michael Paquier <[email protected]> wrote in > > Hi all, > > > > The last TODO item I had in my bucket about the generalization of > > pgstats is the option to a better control on the flush of the stats > > depending on the kind for fixed-numbered stats. Currently, this is > > controlled by pgstat_report_stat(), that includes special handling for > > WAL, IO and SLRU stats, with two generic concepts: > > - Check if there are pending entries, allowing a fast-path exit. > > - Do the actual flush, with a recheck on pending entries. > > > > SLRU and IO control that with one variable each, and WAL uses a > > routine for the same called pgstat_have_pending_wal(). Please find > > attached a patch to generalize the concept, with two new callbacks > > that can be used for fixed-numbered stats. SLRU, IO and WAL are > > switched to use these (the two pgstat_flush_* routines have been kept > > on purpose). This brings some clarity in the code, by making > > have_iostats and have_slrustats static in their respective files. The > > two pgstat_flush_* wrappers do not need a boolean as return result. > > The generalization sounds good to me, and hiding the private flags in > private places also seems good. +1 on the idea. > Regarding pgstat_io_flush_cb, I think it no longer needs to check the > have_iostats variable, and that check should be moved to the new > pgstat_flush_io(). The same applies to pgstat_wal_flush_cb() (and > pgstat_flush_wal()). As for pgstat_slru_flush_cb, it simply doesn't > need the check anymore. Agree. Another option could be to add only one callback (the flush_fixed_cb one) and get rid of the have_fixed_pending_cb one. Then add an extra parameter to the flush_fixed_cb that would only check if there is pending stats (without flushing them). I think those 2 callbacks are highly related that's why I think we could "merge" them, thoughts? Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Add callbacks for fixed-numbered stats flush in pgstats @ 2024-09-04 06:12 Michael Paquier <[email protected]> parent: Bertrand Drouvot <[email protected]> 0 siblings, 1 reply; 7+ messages in thread From: Michael Paquier @ 2024-09-04 06:12 UTC (permalink / raw) To: Bertrand Drouvot <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; [email protected] On Wed, Sep 04, 2024 at 05:28:56AM +0000, Bertrand Drouvot wrote: > On Wed, Sep 04, 2024 at 02:05:46PM +0900, Kyotaro Horiguchi wrote: >> The generalization sounds good to me, and hiding the private flags in >> private places also seems good. > > +1 on the idea. Thanks for the feedback. >> Regarding pgstat_io_flush_cb, I think it no longer needs to check the >> have_iostats variable, and that check should be moved to the new >> pgstat_flush_io(). The same applies to pgstat_wal_flush_cb() (and >> pgstat_flush_wal()). As for pgstat_slru_flush_cb, it simply doesn't >> need the check anymore. > > Agree. Another option could be to add only one callback (the flush_fixed_cb one) > and get rid of the have_fixed_pending_cb one. Then add an extra parameter to the > flush_fixed_cb that would only check if there is pending stats (without > flushing them). I think those 2 callbacks are highly related that's why I > think we could "merge" them, thoughts? I would still value the shortcut that we can use if there is no activity to avoid the clock check with GetCurrentTimestamp(), though, which is why I'd rather stick with two callbacks as that can lead to a much cheaper path. Anyway, I am not sure to follow your point about removing the boolean checks in the flush callbacks. It's surely always a better to never call LWLockAcquire() or LWLockConditionalAcquire() if we don't have to, and we would go through the whole flush dance as long as we detect some stats activity in at least one stats kind. I mean, that's cheap enough to keep around. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Add callbacks for fixed-numbered stats flush in pgstats @ 2024-09-04 06:27 Bertrand Drouvot <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 1 reply; 7+ messages in thread From: Bertrand Drouvot @ 2024-09-04 06:27 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; [email protected] Hi, On Wed, Sep 04, 2024 at 03:12:37PM +0900, Michael Paquier wrote: > On Wed, Sep 04, 2024 at 05:28:56AM +0000, Bertrand Drouvot wrote: > > On Wed, Sep 04, 2024 at 02:05:46PM +0900, Kyotaro Horiguchi wrote: > >> The generalization sounds good to me, and hiding the private flags in > >> private places also seems good. > > > > +1 on the idea. > > Thanks for the feedback. > > >> Regarding pgstat_io_flush_cb, I think it no longer needs to check the > >> have_iostats variable, and that check should be moved to the new > >> pgstat_flush_io(). The same applies to pgstat_wal_flush_cb() (and > >> pgstat_flush_wal()). As for pgstat_slru_flush_cb, it simply doesn't > >> need the check anymore. > > > > Agree. Another option could be to add only one callback (the flush_fixed_cb one) > > and get rid of the have_fixed_pending_cb one. Then add an extra parameter to the > > flush_fixed_cb that would only check if there is pending stats (without > > flushing them). I think those 2 callbacks are highly related that's why I > > think we could "merge" them, thoughts? > > I would still value the shortcut that we can use if there is no > activity to avoid the clock check with GetCurrentTimestamp(), Agree. The idea was to add an additional parameter (say "check_only") to the flush_fixed_cb. If this parameter is set to true then the flush_fixed_cb would do nothing (no flush at all) but return a boolean that would indicate if there is pending stats. In case it returns false, then we could avoid the clock check. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Add callbacks for fixed-numbered stats flush in pgstats @ 2024-09-09 02:20 Michael Paquier <[email protected]> parent: Bertrand Drouvot <[email protected]> 0 siblings, 0 replies; 7+ messages in thread From: Michael Paquier @ 2024-09-09 02:20 UTC (permalink / raw) To: Bertrand Drouvot <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; [email protected] On Wed, Sep 04, 2024 at 06:27:43AM +0000, Bertrand Drouvot wrote: > Agree. The idea was to add an additional parameter (say "check_only") to the > flush_fixed_cb. If this parameter is set to true then the flush_fixed_cb would > do nothing (no flush at all) but return a boolean that would indicate if there > is pending stats. In case it returns false, then we could avoid the clock check. Hmm. It's the same thing in terms of logic, but I am not really convinced that it is a good idea to mix a code path for a sanity check with a code path dealing with the action, particularly considering that there is the nowait option to think about in the flush callback which would require one to think about 4 possible states rather than two possibilities. So that's more error-prone for extension developers, IMO. At the end, I have used my original suggestions for the callbacks. If there are voices in favor of something different, we still have a good chunk of the development and beta cycle for that. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2024-09-09 02:20 UTC | newest] Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2023-10-22 02:22 [PATCH v10 5/7] Row pattern recognition patch (docs). Tatsuo Ishii <[email protected]> 2024-09-03 04:48 Add callbacks for fixed-numbered stats flush in pgstats Michael Paquier <[email protected]> 2024-09-04 05:05 ` Re: Add callbacks for fixed-numbered stats flush in pgstats Kyotaro Horiguchi <[email protected]> 2024-09-04 05:28 ` Re: Add callbacks for fixed-numbered stats flush in pgstats Bertrand Drouvot <[email protected]> 2024-09-04 06:12 ` Re: Add callbacks for fixed-numbered stats flush in pgstats Michael Paquier <[email protected]> 2024-09-04 06:27 ` Re: Add callbacks for fixed-numbered stats flush in pgstats Bertrand Drouvot <[email protected]> 2024-09-09 02:20 ` Re: Add callbacks for fixed-numbered stats flush in pgstats Michael Paquier <[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