public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v11 2/7] Row pattern recognition patch (parse/analysis). 10+ messages / 6 participants [nested] [flat]
* [PATCH v11 2/7] Row pattern recognition patch (parse/analysis). @ 2023-11-08 06:57 Tatsuo Ishii <[email protected]> 0 siblings, 0 replies; 10+ messages in thread From: Tatsuo Ishii @ 2023-11-08 06:57 UTC (permalink / raw) --- src/backend/parser/parse_agg.c | 7 + src/backend/parser/parse_clause.c | 278 +++++++++++++++++++++++++++++- src/backend/parser/parse_expr.c | 4 + src/backend/parser/parse_func.c | 3 + 4 files changed, 291 insertions(+), 1 deletion(-) diff --git a/src/backend/parser/parse_agg.c b/src/backend/parser/parse_agg.c index 9bbad33fbd..28fb5e0d71 100644 --- a/src/backend/parser/parse_agg.c +++ b/src/backend/parser/parse_agg.c @@ -575,6 +575,10 @@ check_agglevels_and_constraints(ParseState *pstate, Node *expr) errkind = true; break; + case EXPR_KIND_RPR_DEFINE: + errkind = true; + break; + /* * There is intentionally no default: case here, so that the * compiler will warn if we add a new ParseExprKind without @@ -964,6 +968,9 @@ transformWindowFuncCall(ParseState *pstate, WindowFunc *wfunc, case EXPR_KIND_CYCLE_MARK: errkind = true; break; + case EXPR_KIND_RPR_DEFINE: + errkind = true; + break; /* * There is intentionally no default: case here, so that the diff --git a/src/backend/parser/parse_clause.c b/src/backend/parser/parse_clause.c index 334b9b42bd..c5d3c10683 100644 --- a/src/backend/parser/parse_clause.c +++ b/src/backend/parser/parse_clause.c @@ -100,7 +100,10 @@ static WindowClause *findWindowClause(List *wclist, const char *name); static Node *transformFrameOffset(ParseState *pstate, int frameOptions, Oid rangeopfamily, Oid rangeopcintype, Oid *inRangeFunc, Node *clause); - +static void transformRPR(ParseState *pstate, WindowClause *wc, WindowDef *windef, List **targetlist); +static List *transformDefineClause(ParseState *pstate, WindowClause *wc, WindowDef *windef, List **targetlist); +static void transformPatternClause(ParseState *pstate, WindowClause *wc, WindowDef *windef); +static List *transformMeasureClause(ParseState *pstate, WindowClause *wc, WindowDef *windef); /* * transformFromClause - @@ -2950,6 +2953,10 @@ transformWindowDefinitions(ParseState *pstate, rangeopfamily, rangeopcintype, &wc->endInRangeFunc, windef->endOffset); + + /* Process Row Pattern Recognition related clauses */ + transformRPR(pstate, wc, windef, targetlist); + wc->runCondition = NIL; wc->winref = winref; @@ -3815,3 +3822,272 @@ transformFrameOffset(ParseState *pstate, int frameOptions, return node; } + +/* + * transformRPR + * Process Row Pattern Recognition related clauses + */ +static void +transformRPR(ParseState *pstate, WindowClause *wc, WindowDef *windef, List **targetlist) +{ + /* + * Window definition exists? + */ + if (windef == NULL) + return; + + /* + * Row Pattern Common Syntax clause exists? + */ + if (windef->rpCommonSyntax == NULL) + return; + + /* Check Frame option. Frame must start at current row */ + if ((wc->frameOptions & FRAMEOPTION_START_CURRENT_ROW) == 0) + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("FRAME must start at current row when row patttern recognition is used"))); + + /* Transform AFTER MACH SKIP TO clause */ + wc->rpSkipTo = windef->rpCommonSyntax->rpSkipTo; + + /* Transform SEEK or INITIAL clause */ + wc->initial = windef->rpCommonSyntax->initial; + + /* Transform DEFINE clause into list of TargetEntry's */ + wc->defineClause = transformDefineClause(pstate, wc, windef, targetlist); + + /* Check PATTERN clause and copy to patternClause */ + transformPatternClause(pstate, wc, windef); + + /* Transform MEASURE clause */ + transformMeasureClause(pstate, wc, windef); +} + +/* + * transformDefineClause Process DEFINE clause and transform ResTarget into + * list of TargetEntry. + * + * XXX we only support column reference in row pattern definition search + * condition, e.g. "price". <row pattern definition variable name>.<column + * reference> is not supported, e.g. "A.price". + */ +static List * +transformDefineClause(ParseState *pstate, WindowClause *wc, WindowDef *windef, List **targetlist) +{ + /* DEFINE variable name initials */ + static char *defineVariableInitials = "abcdefghijklmnopqrstuvwxyz"; + + ListCell *lc, *l; + ResTarget *restarget, *r; + List *restargets; + List *defineClause; + char *name; + int initialLen; + int i; + + /* + * If Row Definition Common Syntax exists, DEFINE clause must exist. + * (the raw parser should have already checked it.) + */ + Assert(windef->rpCommonSyntax->rpDefs != NULL); + + /* + * Check and add "A AS A IS TRUE" if pattern variable is missing in DEFINE + * per the SQL standard. + */ + restargets = NIL; + foreach(lc, windef->rpCommonSyntax->rpPatterns) + { + A_Expr *a; + bool found = false; + + if (!IsA(lfirst(lc), A_Expr)) + ereport(ERROR, + errmsg("node type is not A_Expr")); + + a = (A_Expr *)lfirst(lc); + name = strVal(a->lexpr); + + foreach(l, windef->rpCommonSyntax->rpDefs) + { + restarget = (ResTarget *)lfirst(l); + + if (!strcmp(restarget->name, name)) + { + found = true; + break; + } + } + + if (!found) + { + /* + * "name" is missing. So create "name AS name IS TRUE" ResTarget + * node and add it to the temporary list. + */ + A_Const *n; + + restarget = makeNode(ResTarget); + n = makeNode(A_Const); + n->val.boolval.type = T_Boolean; + n->val.boolval.boolval = true; + n->location = -1; + restarget->name = pstrdup(name); + restarget->indirection = NIL; + restarget->val = (Node *)n; + restarget->location = -1; + restargets = lappend((List *)restargets, restarget); + } + } + + if (list_length(restargets) >= 1) + { + /* add missing DEFINEs */ + windef->rpCommonSyntax->rpDefs = list_concat(windef->rpCommonSyntax->rpDefs, + restargets); + list_free(restargets); + } + + /* + * Check for duplicate row pattern definition variables. The standard + * requires that no two row pattern definition variable names shall be + * equivalent. + */ + restargets = NIL; + foreach(lc, windef->rpCommonSyntax->rpDefs) + { + restarget = (ResTarget *)lfirst(lc); + name = restarget->name; + + /* + * Add DEFINE expression (Restarget->val) to the targetlist as a + * TargetEntry if it does not exist yet. Planner will add the column + * ref var node to the outer plan's target list later on. This makes + * DEFINE expression could access the outer tuple while evaluating + * PATTERN. + * + * XXX: adding whole expressions of DEFINE to the plan.targetlist is + * not so good, because it's not necessary to evalute the expression + * in the target list while running the plan. We should extract the + * var nodes only then add them to the plan.targetlist. + */ + findTargetlistEntrySQL99(pstate, (Node *)restarget->val, targetlist, EXPR_KIND_RPR_DEFINE); + + /* + * Make sure that the row pattern definition search condition is a + * boolean expression. + */ + transformWhereClause(pstate, restarget->val, + EXPR_KIND_RPR_DEFINE, "DEFINE"); + + foreach(l, restargets) + { + char *n; + + r = (ResTarget *) lfirst(l); + n = r->name; + + if (!strcmp(n, name)) + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("row pattern definition variable name \"%s\" appears more than once in DEFINE clause", + name), + parser_errposition(pstate, exprLocation((Node *)r)))); + } + restargets = lappend(restargets, restarget); + } + list_free(restargets); + + /* + * Create list of row pattern DEFINE variable name's initial. + * We assign [a-z] to them (up to 26 variable names are allowed). + */ + restargets = NIL; + i = 0; + initialLen = strlen(defineVariableInitials); + + foreach(lc, windef->rpCommonSyntax->rpDefs) + { + char initial[2]; + + restarget = (ResTarget *)lfirst(lc); + name = restarget->name; + + if (i >= initialLen) + { + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("number of row pattern definition variable names exceeds %d", initialLen), + parser_errposition(pstate, exprLocation((Node *)restarget)))); + } + initial[0] = defineVariableInitials[i++]; + initial[1] = '\0'; + wc->defineInitial = lappend(wc->defineInitial, makeString(pstrdup(initial))); + } + + defineClause = transformTargetList(pstate, windef->rpCommonSyntax->rpDefs, + EXPR_KIND_RPR_DEFINE); + + /* mark column origins */ + markTargetListOrigins(pstate, defineClause); + + /* mark all nodes in the DEFINE clause tree with collation information */ + assign_expr_collations(pstate, (Node *)defineClause); + + return defineClause; +} + +/* + * transformPatternClause + * Process PATTERN clause and return PATTERN clause in the raw parse tree + */ +static void +transformPatternClause(ParseState *pstate, WindowClause *wc, WindowDef *windef) +{ + ListCell *lc; + + /* + * Row Pattern Common Syntax clause exists? + */ + if (windef->rpCommonSyntax == NULL) + return; + + wc->patternVariable = NIL; + wc->patternRegexp = NIL; + foreach(lc, windef->rpCommonSyntax->rpPatterns) + { + A_Expr *a; + char *name; + char *regexp; + + if (!IsA(lfirst(lc), A_Expr)) + ereport(ERROR, + errmsg("node type is not A_Expr")); + + a = (A_Expr *)lfirst(lc); + name = strVal(a->lexpr); + + wc->patternVariable = lappend(wc->patternVariable, makeString(pstrdup(name))); + regexp = strVal(lfirst(list_head(a->name))); + wc->patternRegexp = lappend(wc->patternRegexp, makeString(pstrdup(regexp))); + } +} + +/* + * transformMeasureClause + * Process MEASURE clause + * XXX MEASURE clause is not supported yet + */ +static List * +transformMeasureClause(ParseState *pstate, WindowClause *wc, WindowDef *windef) +{ + if (windef->rowPatternMeasures == NIL) + return NIL; + + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("%s","MEASURE clause is not supported yet"), + parser_errposition(pstate, exprLocation((Node *)windef->rowPatternMeasures)))); + return NIL; +} diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c index 64c582c344..18b58ac263 100644 --- a/src/backend/parser/parse_expr.c +++ b/src/backend/parser/parse_expr.c @@ -557,6 +557,7 @@ transformColumnRef(ParseState *pstate, ColumnRef *cref) case EXPR_KIND_COPY_WHERE: case EXPR_KIND_GENERATED_COLUMN: case EXPR_KIND_CYCLE_MARK: + case EXPR_KIND_RPR_DEFINE: /* okay */ break; @@ -1770,6 +1771,7 @@ transformSubLink(ParseState *pstate, SubLink *sublink) case EXPR_KIND_VALUES: case EXPR_KIND_VALUES_SINGLE: case EXPR_KIND_CYCLE_MARK: + case EXPR_KIND_RPR_DEFINE: /* okay */ break; case EXPR_KIND_CHECK_CONSTRAINT: @@ -3149,6 +3151,8 @@ ParseExprKindName(ParseExprKind exprKind) return "GENERATED AS"; case EXPR_KIND_CYCLE_MARK: return "CYCLE"; + case EXPR_KIND_RPR_DEFINE: + return "DEFINE"; /* * There is intentionally no default: case here, so that the diff --git a/src/backend/parser/parse_func.c b/src/backend/parser/parse_func.c index 6c29471bb3..086431f91b 100644 --- a/src/backend/parser/parse_func.c +++ b/src/backend/parser/parse_func.c @@ -2656,6 +2656,9 @@ check_srf_call_placement(ParseState *pstate, Node *last_srf, int location) case EXPR_KIND_CYCLE_MARK: errkind = true; break; + case EXPR_KIND_RPR_DEFINE: + errkind = true; + break; /* * There is intentionally no default: case here, so that the -- 2.25.1 ----Next_Part(Wed_Nov__8_16_37_05_2023_872)-- Content-Type: Text/X-Patch; charset=us-ascii Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="v11-0003-Row-pattern-recognition-patch-planner.patch" ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: PoC: history of recent vacuum/checkpoint runs (using new hooks) @ 2024-12-26 16:00 wenhui qiu <[email protected]> 0 siblings, 1 reply; 10+ messages in thread From: wenhui qiu @ 2024-12-26 16:00 UTC (permalink / raw) To: Robert Treat <[email protected]>; +Cc: Tomas Vondra <[email protected]>; pgsql-hackers Hi As far as I know, more than 10,000 tables of instances are often encountered, So I insist that the maximum can be appropriately increased to 256MB，Can be more adaptable to many table situations On Thu, 26 Dec 2024 at 23:23, Robert Treat <[email protected]> wrote: > On Wed, Dec 25, 2024 at 12:26 PM Tomas Vondra <[email protected]> wrote: > > > > Hi, > > > > On 12/23/24 07:35, wenhui qiu wrote: > > > Hi Tomas > > > This is a great feature. > > > + /* > > > + * Define (or redefine) custom GUC variables. > > > + */ > > > + DefineCustomIntVariable("stats_history.size", > > > + "Sets the amount of memory available for past events.", > > > + NULL, > > > + &statsHistorySizeMB, > > > + 1, > > > + 1, > > > + 128, > > > + PGC_POSTMASTER, > > > + GUC_UNIT_MB, > > > + NULL, > > > + NULL, > > > + NULL); > > > + > > > RAM is in terabytes now, the statsHistorySize is 128MB ,I think can > > > increase to store more history record ? > > > > > > > Maybe, the 128MB is an arbitrary (and conservative) limit - it's enough > > for ~500k events, which seems good enough for most systems. Of course, > > on systems with many relations might need more space, not sure. > > > > I was thinking about specifying the space in more natural terms, either > > as amount of time ("keep 1 day of history") or number of entries ("10k > > entries"). That would probably mean the memory can't be allocated as > > fixed size. > > > > Based on the above, a rough calculation is that this is enough for > holding 1 year of hourly vacuum runs for 50 tables, or a year of daily > vacuums for 1000 tables. Most folks will fall somewhere in that range > (and won't really need a year's history) but that seems like plenty > for a default. > > > But maybe it'd be possible to just write the entries to a file. We don't > > need random access to past entries (unlike e.g. pg_stat_statements), and > > people won't query that very often either. > > > > Yeah, workloads will vary, but it doesn't seem like they would more > than query workloads do. > > Robert Treat > https://xzilla.net > ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: PoC: history of recent vacuum/checkpoint runs (using new hooks) @ 2024-12-26 17:58 Tomas Vondra <[email protected]> parent: wenhui qiu <[email protected]> 0 siblings, 1 reply; 10+ messages in thread From: Tomas Vondra @ 2024-12-26 17:58 UTC (permalink / raw) To: wenhui qiu <[email protected]>; Robert Treat <[email protected]>; +Cc: pgsql-hackers On 12/26/24 17:00, wenhui qiu wrote: > Hi > > As far as I know, more than 10,000 tables of instances are often > encountered, > So I insist that the maximum can be appropriately increased to 256MB， > Can be more adaptable to many table situations > If 128MB is insufficient, why would 256MB be OK? A factor of 2x does not make a fundamental difference ... Anyway, the 128MB value is rather arbitrary. I don't mind increasing the limit, or possibly removing it entirely (and accepting anything the system can handle). regards -- Tomas Vondra ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: PoC: history of recent vacuum/checkpoint runs (using new hooks) @ 2024-12-27 04:00 Michael Paquier <[email protected]> parent: Tomas Vondra <[email protected]> 0 siblings, 1 reply; 10+ messages in thread From: Michael Paquier @ 2024-12-27 04:00 UTC (permalink / raw) To: Tomas Vondra <[email protected]>; +Cc: wenhui qiu <[email protected]>; Robert Treat <[email protected]>; pgsql-hackers On Thu, Dec 26, 2024 at 06:58:11PM +0100, Tomas Vondra wrote: > If 128MB is insufficient, why would 256MB be OK? A factor of 2x does not > make a fundamental difference ... > > Anyway, the 128MB value is rather arbitrary. I don't mind increasing the > limit, or possibly removing it entirely (and accepting anything the > system can handle). + DefineCustomIntVariable("stats_history.size", + "Sets the amount of memory available for past events.", How about some time-based retention? Data size can be hard to think about for the end user, while there would be a set of users that would want to retain data for the past week, month, etc? If both size and time upper-bound are define, then entries that match one condition or the other are removed. + checkpoint_log_hook( + CheckpointStats.ckpt_start_t, /* start_time */ + CheckpointStats.ckpt_end_t, /* end_time */ + (flags & CHECKPOINT_IS_SHUTDOWN), /* is_shutdown */ + (flags & CHECKPOINT_END_OF_RECOVERY), /* is_end_of_recovery */ + (flags & CHECKPOINT_IMMEDIATE), /* is_immediate */ + (flags & CHECKPOINT_FORCE), /* is_force */ + (flags & CHECKPOINT_WAIT), /* is_wait */ + (flags & CHECKPOINT_CAUSE_XLOG), /* is_wal */ + (flags & CHECKPOINT_CAUSE_TIME), /* is_time */ + (flags & CHECKPOINT_FLUSH_ALL), /* is_flush_all */ + CheckpointStats.ckpt_bufs_written, /* buffers_written */ + CheckpointStats.ckpt_slru_written, /* slru_written */ + CheckpointStats.ckpt_segs_added, /* segs_added */ + CheckpointStats.ckpt_segs_removed, /* segs_removed */ + CheckpointStats.ckpt_segs_recycled, /* segs_recycled */ That's a lot of arguments. CheckpointStatsData and the various CHECKPOINT_* flags are exposed, why not just send these values to the hook? For v1-0001 as well, I'd suggest some grouping with existing structures, or expose these structures so as they can be reused for out-of-core code via the proposed hook. More arguments lead to more mistakes that could be easily avoided. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: PoC: history of recent vacuum/checkpoint runs (using new hooks) @ 2024-12-28 01:25 Tomas Vondra <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 2 replies; 10+ messages in thread From: Tomas Vondra @ 2024-12-28 01:25 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: wenhui qiu <[email protected]>; Robert Treat <[email protected]>; pgsql-hackers On 12/27/24 05:00, Michael Paquier wrote: > On Thu, Dec 26, 2024 at 06:58:11PM +0100, Tomas Vondra wrote: >> If 128MB is insufficient, why would 256MB be OK? A factor of 2x does not >> make a fundamental difference ... >> >> Anyway, the 128MB value is rather arbitrary. I don't mind increasing the >> limit, or possibly removing it entirely (and accepting anything the >> system can handle). > > + DefineCustomIntVariable("stats_history.size", > + "Sets the amount of memory available for past events.", > > How about some time-based retention? Data size can be hard to think > about for the end user, while there would be a set of users that would > want to retain data for the past week, month, etc? If both size and > time upper-bound are define, then entries that match one condition or > the other are removed. > Right. In my response [1] I suggested replacing the simple memory limit with a time-based limit, but I haven't done anything about that yet. And the more I think about it the more I'm convinced we don't need to keep the data about past runs in memory, a file should be enough (except maybe for a small buffer). That would mean we don't need to worry about dynamic shared memory, etc. I initially rejected this because it seemed like a regression to how pgstat worked initially (sharing data through files), but I don't think that's actually true - this data is different (almost append-only), etc. The one case when we may need co read the data is in response to DROP of a table, when we need to discard entries for that object. Or we could handle that by recording OIDs of dropped objects ... not sure how complex this would need to be. We'd still want to enforce some limits, of course - a time-based limit of the minimum amount of time to cover, and maximum amount of disk space to use (more as a safety). FWIW there's one "issue" with enforcing the time-based limit - we can only do that for the "end time", because that's when we see the entry. If you configure the limit to keep "1 day" history, and then a vacuum completes after running for 2 days, we want to keep it, so that anyone can actually see it. [1] https://www.postgresql.org/message-id/8df7cee1-31aa-4db3-bbb7-83157ca139da%40vondra.me > + checkpoint_log_hook( > + CheckpointStats.ckpt_start_t, /* start_time */ > + CheckpointStats.ckpt_end_t, /* end_time */ > + (flags & CHECKPOINT_IS_SHUTDOWN), /* is_shutdown */ > + (flags & CHECKPOINT_END_OF_RECOVERY), /* is_end_of_recovery */ > + (flags & CHECKPOINT_IMMEDIATE), /* is_immediate */ > + (flags & CHECKPOINT_FORCE), /* is_force */ > + (flags & CHECKPOINT_WAIT), /* is_wait */ > + (flags & CHECKPOINT_CAUSE_XLOG), /* is_wal */ > + (flags & CHECKPOINT_CAUSE_TIME), /* is_time */ > + (flags & CHECKPOINT_FLUSH_ALL), /* is_flush_all */ > + CheckpointStats.ckpt_bufs_written, /* buffers_written */ > + CheckpointStats.ckpt_slru_written, /* slru_written */ > + CheckpointStats.ckpt_segs_added, /* segs_added */ > + CheckpointStats.ckpt_segs_removed, /* segs_removed */ > + CheckpointStats.ckpt_segs_recycled, /* segs_recycled */ > > That's a lot of arguments. CheckpointStatsData and the various > CHECKPOINT_* flags are exposed, why not just send these values to the > hook? > > For v1-0001 as well, I'd suggest some grouping with existing > structures, or expose these structures so as they can be reused for > out-of-core code via the proposed hook. More arguments lead to more > mistakes that could be easily avoided. Yes, I admit the number of parameters seemed a bit annoying to me too, and maybe we could reduce that somewhat. Certainly for checkpoints, where we already have a reasonable CheckpointStats struct and flags, wrapping most of the fields. With vacuum it's a bit more complicated, for two reasons: (a) the counters are simply in LVRelState, mixed with all kinds of other info, and it seems "not great" to pass it to a "log" hook, and (b) there are some calculated values, so I guess the hook would need to do that calculation on it's own, and it'd be easy to diverge over time. For (a) we could introduce some "stats" struct to keep these counters for vacuum (the nearby parallel vacuum patch actually does something like that, I think). Not sure if (b) is actually a problem in practice, but I guess we could add those fields to the new "stats" struct too. regards -- Tomas Vondra ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: PoC: history of recent vacuum/checkpoint runs (using new hooks) @ 2024-12-29 15:39 Robert Treat <[email protected]> parent: Tomas Vondra <[email protected]> 1 sibling, 1 reply; 10+ messages in thread From: Robert Treat @ 2024-12-29 15:39 UTC (permalink / raw) To: Tomas Vondra <[email protected]>; +Cc: Michael Paquier <[email protected]>; wenhui qiu <[email protected]>; pgsql-hackers On Fri, Dec 27, 2024 at 8:25 PM Tomas Vondra <[email protected]> wrote: > On 12/27/24 05:00, Michael Paquier wrote: > > On Thu, Dec 26, 2024 at 06:58:11PM +0100, Tomas Vondra wrote: > >> If 128MB is insufficient, why would 256MB be OK? A factor of 2x does not > >> make a fundamental difference ... > >> > >> Anyway, the 128MB value is rather arbitrary. I don't mind increasing the > >> limit, or possibly removing it entirely (and accepting anything the > >> system can handle). > > > > + DefineCustomIntVariable("stats_history.size", > > + "Sets the amount of memory available for past events.", > > > > How about some time-based retention? Data size can be hard to think > > about for the end user, while there would be a set of users that would > > want to retain data for the past week, month, etc? If both size and > > time upper-bound are define, then entries that match one condition or > > the other are removed. > > > > Right. In my response [1] I suggested replacing the simple memory limit > with a time-based limit, but I haven't done anything about that yet. > > And the more I think about it the more I'm convinced we don't need to > keep the data about past runs in memory, a file should be enough (except > maybe for a small buffer). That would mean we don't need to worry about > dynamic shared memory, etc. I initially rejected this because it seemed > like a regression to how pgstat worked initially (sharing data through > files), but I don't think that's actually true - this data is different > (almost append-only), etc. > > The one case when we may need co read the data is in response to DROP of > a table, when we need to discard entries for that object. Or we could > handle that by recording OIDs of dropped objects ... not sure how > complex this would need to be. > > We'd still want to enforce some limits, of course - a time-based limit > of the minimum amount of time to cover, and maximum amount of disk space > to use (more as a safety). > > FWIW there's one "issue" with enforcing the time-based limit - we can > only do that for the "end time", because that's when we see the entry. > If you configure the limit to keep "1 day" history, and then a vacuum > completes after running for 2 days, we want to keep it, so that anyone > can actually see it. > I can't say I recall all the reasoning involved in making pg_stat_statements just be based on a fixed number of entries, but the ability to come up with corner cases was certainly a factor. For example, imagine the scenario where you set a max at 30 days, but you have some tables only being vacuumed every few months. Ideally you probably want the last entry no matter what, and honestly probably the last 2 (in case you are troubleshooting something, having the last run and something to compare against is ideal). In any case, it can get complicated pretty quickly. > [1] > https://www.postgresql.org/message-id/8df7cee1-31aa-4db3-bbb7-83157ca139da%40vondra.me > > > + checkpoint_log_hook( > > + CheckpointStats.ckpt_start_t, /* start_time */ > > + CheckpointStats.ckpt_end_t, /* end_time */ > > + (flags & CHECKPOINT_IS_SHUTDOWN), /* is_shutdown */ > > + (flags & CHECKPOINT_END_OF_RECOVERY), /* is_end_of_recovery */ > > + (flags & CHECKPOINT_IMMEDIATE), /* is_immediate */ > > + (flags & CHECKPOINT_FORCE), /* is_force */ > > + (flags & CHECKPOINT_WAIT), /* is_wait */ > > + (flags & CHECKPOINT_CAUSE_XLOG), /* is_wal */ > > + (flags & CHECKPOINT_CAUSE_TIME), /* is_time */ > > + (flags & CHECKPOINT_FLUSH_ALL), /* is_flush_all */ > > + CheckpointStats.ckpt_bufs_written, /* buffers_written */ > > + CheckpointStats.ckpt_slru_written, /* slru_written */ > > + CheckpointStats.ckpt_segs_added, /* segs_added */ > > + CheckpointStats.ckpt_segs_removed, /* segs_removed */ > > + CheckpointStats.ckpt_segs_recycled, /* segs_recycled */ > > > > That's a lot of arguments. CheckpointStatsData and the various > > CHECKPOINT_* flags are exposed, why not just send these values to the > > hook? > > > > For v1-0001 as well, I'd suggest some grouping with existing > > structures, or expose these structures so as they can be reused for > > out-of-core code via the proposed hook. More arguments lead to more > > mistakes that could be easily avoided. > > Yes, I admit the number of parameters seemed a bit annoying to me too, > and maybe we could reduce that somewhat. Certainly for checkpoints, > where we already have a reasonable CheckpointStats struct and flags, > wrapping most of the fields. > > With vacuum it's a bit more complicated, for two reasons: (a) the > counters are simply in LVRelState, mixed with all kinds of other info, > and it seems "not great" to pass it to a "log" hook, and (b) there are > some calculated values, so I guess the hook would need to do that > calculation on it's own, and it'd be easy to diverge over time. > > For (a) we could introduce some "stats" struct to keep these counters > for vacuum (the nearby parallel vacuum patch actually does something > like that, I think). Not sure if (b) is actually a problem in practice, > but I guess we could add those fields to the new "stats" struct too. > At the risk of increasing scope, since you already are working on checkpoints along with vacuums, I'm curious if there was a reason not to do analyze stats retention as well? It seems pretty correlated in the same area/problems as vacuum history. Robert Treat https://xzilla.net ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: PoC: history of recent vacuum/checkpoint runs (using new hooks) @ 2024-12-29 18:25 Tomas Vondra <[email protected]> parent: Robert Treat <[email protected]> 0 siblings, 0 replies; 10+ messages in thread From: Tomas Vondra @ 2024-12-29 18:25 UTC (permalink / raw) To: Robert Treat <[email protected]>; +Cc: Michael Paquier <[email protected]>; wenhui qiu <[email protected]>; pgsql-hackers On 12/29/24 16:39, Robert Treat wrote: > On Fri, Dec 27, 2024 at 8:25 PM Tomas Vondra <[email protected]> wrote: >> On 12/27/24 05:00, Michael Paquier wrote: >>> On Thu, Dec 26, 2024 at 06:58:11PM +0100, Tomas Vondra wrote: >>>> If 128MB is insufficient, why would 256MB be OK? A factor of 2x does not >>>> make a fundamental difference ... >>>> >>>> Anyway, the 128MB value is rather arbitrary. I don't mind increasing the >>>> limit, or possibly removing it entirely (and accepting anything the >>>> system can handle). >>> >>> + DefineCustomIntVariable("stats_history.size", >>> + "Sets the amount of memory available for past events.", >>> >>> How about some time-based retention? Data size can be hard to think >>> about for the end user, while there would be a set of users that would >>> want to retain data for the past week, month, etc? If both size and >>> time upper-bound are define, then entries that match one condition or >>> the other are removed. >>> >> >> Right. In my response [1] I suggested replacing the simple memory limit >> with a time-based limit, but I haven't done anything about that yet. >> >> And the more I think about it the more I'm convinced we don't need to >> keep the data about past runs in memory, a file should be enough (except >> maybe for a small buffer). That would mean we don't need to worry about >> dynamic shared memory, etc. I initially rejected this because it seemed >> like a regression to how pgstat worked initially (sharing data through >> files), but I don't think that's actually true - this data is different >> (almost append-only), etc. >> >> The one case when we may need co read the data is in response to DROP of >> a table, when we need to discard entries for that object. Or we could >> handle that by recording OIDs of dropped objects ... not sure how >> complex this would need to be. >> >> We'd still want to enforce some limits, of course - a time-based limit >> of the minimum amount of time to cover, and maximum amount of disk space >> to use (more as a safety). >> >> FWIW there's one "issue" with enforcing the time-based limit - we can >> only do that for the "end time", because that's when we see the entry. >> If you configure the limit to keep "1 day" history, and then a vacuum >> completes after running for 2 days, we want to keep it, so that anyone >> can actually see it. >> > > I can't say I recall all the reasoning involved in making > pg_stat_statements just be based on a fixed number of entries, but the > ability to come up with corner cases was certainly a factor. For > example, imagine the scenario where you set a max at 30 days, but you > have some tables only being vacuumed every few months. Ideally you > probably want the last entry no matter what, and honestly probably the > last 2 (in case you are troubleshooting something, having the last run > and something to compare against is ideal). In any case, it can get > complicated pretty quickly. > I really don't want to get into such complicated retention policies, because there's no "right" solution, and it adds complexity both to implementation (e.g. we'd need to keep per-relation counters for all the various events), and processing (How would you know if there were no other vacuum runs on a relation or if those other events were removed?) I think the best solution to use a trivial retention policy (e.g. keep everything for X days), and if you want to keep a longer history, you need to read and archive that regularly (and the retention period ensures you don't miss any events). After all, that's what we assume for most other runtime stats anyway - it's not very useful to know the current values, if you can't calculate deltas. So you have to sample the stats. There's also the trouble that this is not crash safe, so I'd hesitate to rely on this very much if it can disappear. > ... > > At the risk of increasing scope, since you already are working on > checkpoints along with vacuums, I'm curious if there was a reason not > to do analyze stats retention as well? It seems pretty correlated in > the same area/problems as vacuum history. > I agree tracking ANALYZE would be useful. I ignored it mostly to keep the scope as limited as possible, so two separate events were enough. Adding another hook to do_analyze_rel() would be fairly trivial, but I'll leave that for later. regards -- Tomas Vondra ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: PoC: history of recent vacuum/checkpoint runs (using new hooks) @ 2024-12-31 01:06 Michael Paquier <[email protected]> parent: Tomas Vondra <[email protected]> 1 sibling, 1 reply; 10+ messages in thread From: Michael Paquier @ 2024-12-31 01:06 UTC (permalink / raw) To: Tomas Vondra <[email protected]>; +Cc: wenhui qiu <[email protected]>; Robert Treat <[email protected]>; pgsql-hackers On Sat, Dec 28, 2024 at 02:25:16AM +0100, Tomas Vondra wrote: > And the more I think about it the more I'm convinced we don't need to > keep the data about past runs in memory, a file should be enough (except > maybe for a small buffer). That would mean we don't need to worry about > dynamic shared memory, etc. I initially rejected this because it seemed > like a regression to how pgstat worked initially (sharing data through > files), but I don't think that's actually true - this data is different > (almost append-only), etc. Right, I was looking a bit at 0003 that introduces the extension. I am wondering if we are not repeating the errors of pgss by using a different file, and if we should not just use pgstats and its single file instead to store this data through an extension. You are right that as an append-only pattern using the dshash of pgstat does not fit well into this picture. How about the second type of stats kinds: the fixed-numbered stats kind? These allocate a fixed amount of shared memory, meaning that you could allocate N entries of history and just manage a queue of them, then do a memcpy() of the whole set if adding new history at the head of the queue, or just append new ones at the tail of the queue in shmem, memcpy() once the queue is full. The extension gets simpler: - No need to manage a new file, flush of the stats is controlled by pgstats itself. - The extension could register a fixed-numbered custom stats kind. - Push the stats with the new hooks. - Perhaps less useful, but it is possible to control the timing where the data is pushed. - Add SQL wrappers on top to fetch the data from pgstat. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: PoC: history of recent vacuum/checkpoint runs (using new hooks) @ 2024-12-31 15:06 Tomas Vondra <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 1 reply; 10+ messages in thread From: Tomas Vondra @ 2024-12-31 15:06 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: wenhui qiu <[email protected]>; Robert Treat <[email protected]>; pgsql-hackers On 12/31/24 02:06, Michael Paquier wrote: > On Sat, Dec 28, 2024 at 02:25:16AM +0100, Tomas Vondra wrote: >> And the more I think about it the more I'm convinced we don't need to >> keep the data about past runs in memory, a file should be enough (except >> maybe for a small buffer). That would mean we don't need to worry about >> dynamic shared memory, etc. I initially rejected this because it seemed >> like a regression to how pgstat worked initially (sharing data through >> files), but I don't think that's actually true - this data is different >> (almost append-only), etc. > > Right, I was looking a bit at 0003 that introduces the extension. I > am wondering if we are not repeating the errors of pgss by using a > different file, and if we should not just use pgstats and its single > file instead to store this data through an extension. You are right > that as an append-only pattern using the dshash of pgstat does not fit > well into this picture. How about the second type of stats kinds: > the fixed-numbered stats kind? These allocate a fixed amount of > shared memory, meaning that you could allocate N entries of history > and just manage a queue of them, then do a memcpy() of the whole set > if adding new history at the head of the queue, or just append new > ones at the tail of the queue in shmem, memcpy() once the queue is > full. The extension gets simpler: > - No need to manage a new file, flush of the stats is controlled by > pgstats itself. > - The extension could register a fixed-numbered custom stats kind. I'm not against leveraging some of the existing pstat infrastructure, but as I explained earlier I don't like the "fixed amount of shmem" approach. It's either wasteful (on machines with few vacuum runs) or difficult to configure to keep enough history. FWIW I'm not sure what you mean by "pgss errors" - I'm not saying it's flawless, but OTOH reading/writing a flat file with entries is not particularly hard. (It'd need to be a bit more complex to evict stuff for dropped objects, but not by much). > - Push the stats with the new hooks. > - Perhaps less useful, but it is possible to control the timing where > the data is pushed. Not sure what you mean by "pushed". Pushed where, or how would that matter? > - Add SQL wrappers on top to fetch the data from pgstat. OK, although I think this is not particularly complicated part of the code. It could even be simplified further. regards -- Tomas Vondra ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: PoC: history of recent vacuum/checkpoint runs (using new hooks) @ 2025-01-07 09:47 Cédric Villemain <[email protected]> parent: Tomas Vondra <[email protected]> 0 siblings, 0 replies; 10+ messages in thread From: Cédric Villemain @ 2025-01-07 09:47 UTC (permalink / raw) To: Tomas Vondra <[email protected]>; Michael Paquier <[email protected]>; +Cc: wenhui qiu <[email protected]>; Robert Treat <[email protected]>; pgsql-hackers On 31/12/2024 16:06, Tomas Vondra wrote: > On 12/31/24 02:06, Michael Paquier wrote: >> On Sat, Dec 28, 2024 at 02:25:16AM +0100, Tomas Vondra wrote: >>> And the more I think about it the more I'm convinced we don't need to >>> keep the data about past runs in memory, a file should be enough (except >>> maybe for a small buffer). That would mean we don't need to worry about >>> dynamic shared memory, etc. I initially rejected this because it seemed >>> like a regression to how pgstat worked initially (sharing data through >>> files), but I don't think that's actually true - this data is different >>> (almost append-only), etc. >> Right, I was looking a bit at 0003 that introduces the extension. I >> am wondering if we are not repeating the errors of pgss by using a >> different file, and if we should not just use pgstats and its single >> file instead to store this data through an extension. You are right >> that as an append-only pattern using the dshash of pgstat does not fit >> well into this picture. How about the second type of stats kinds: >> the fixed-numbered stats kind? These allocate a fixed amount of >> shared memory, meaning that you could allocate N entries of history >> and just manage a queue of them, then do a memcpy() of the whole set >> if adding new history at the head of the queue, or just append new >> ones at the tail of the queue in shmem, memcpy() once the queue is >> full. The extension gets simpler: >> - No need to manage a new file, flush of the stats is controlled by >> pgstats itself. >> - The extension could register a fixed-numbered custom stats kind. > > I'm not against leveraging some of the existing pstat infrastructure, > but as I explained earlier I don't like the "fixed amount of shmem" > approach. It's either wasteful (on machines with few vacuum runs) or > difficult to configure to keep enough history. A bit late on this topic, I wrote StatsMgr which run stats snapshots based on interval, it covers only fixed stats at the moment though, it's ring buffer too, not perfect but does the job. It has many similarities and I though it'll be interesting to share what I was looking at to improve further. I think it'll be interesting to have 2 hooks in PgStat_KindInfo functions call back: * "flush_fixed_cb()": a hook inside to be able to execute other action while flushing, like generating aggregates ("counter summary") * a new member to do the snapshot directly (or whatever action relevant when the stats are in an interesting state), maybe snapshot_fixed_cb(), with a hook inside. This way, with 2 hooks we can cover both usages, for all stats (fixed here, but probably the same hooks for variable stats), and have actions on events instead of only on time interval. I don't have strict opinion about files management but having a facility from PostgreSQL to store *metrics* (as opposed to "stats" which are required for PostgreSQL) will be very convenient. Maybe like SLRU or similar (the thing used to keep commits timestamp) ? I didn't checked all already available options in this area. [1] https://codeberg.org/Data-Bene/StatsMgr --- Cédric Villemain +33 6 20 30 22 52 https://www.Data-Bene.io PostgreSQL Support, Expertise, Training, R&D ^ permalink raw reply [nested|flat] 10+ messages in thread
end of thread, other threads:[~2025-01-07 09:47 UTC | newest] Thread overview: 10+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2023-11-08 06:57 [PATCH v11 2/7] Row pattern recognition patch (parse/analysis). Tatsuo Ishii <[email protected]> 2024-12-26 16:00 Re: PoC: history of recent vacuum/checkpoint runs (using new hooks) wenhui qiu <[email protected]> 2024-12-26 17:58 ` Re: PoC: history of recent vacuum/checkpoint runs (using new hooks) Tomas Vondra <[email protected]> 2024-12-27 04:00 ` Re: PoC: history of recent vacuum/checkpoint runs (using new hooks) Michael Paquier <[email protected]> 2024-12-28 01:25 ` Re: PoC: history of recent vacuum/checkpoint runs (using new hooks) Tomas Vondra <[email protected]> 2024-12-29 15:39 ` Re: PoC: history of recent vacuum/checkpoint runs (using new hooks) Robert Treat <[email protected]> 2024-12-29 18:25 ` Re: PoC: history of recent vacuum/checkpoint runs (using new hooks) Tomas Vondra <[email protected]> 2024-12-31 01:06 ` Re: PoC: history of recent vacuum/checkpoint runs (using new hooks) Michael Paquier <[email protected]> 2024-12-31 15:06 ` Re: PoC: history of recent vacuum/checkpoint runs (using new hooks) Tomas Vondra <[email protected]> 2025-01-07 09:47 ` Re: PoC: history of recent vacuum/checkpoint runs (using new hooks) Cédric Villemain <[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