public inbox for [email protected]help / color / mirror / Atom feed
[PATCH 1/1] Add high key "continuescan" optimization. 32+ messages / 5 participants [nested] [flat]
* [PATCH 1/1] Add high key "continuescan" optimization. @ 2018-11-12 21:11 Peter Geoghegan <[email protected]> 0 siblings, 0 replies; 32+ messages in thread From: Peter Geoghegan @ 2018-11-12 21:11 UTC (permalink / raw) Teach B-Tree forward index scans to check the high key before moving to the next page in the hopes of finding that it isn't actually necessary to move to the next page. We already opportunistically force a key check of the last item on leaf pages, even when it's clear that it cannot be returned to the scan due to being dead-to-all, for the same reason. Since forcing the last item to be key checked no longer makes any difference in the case of forward scans, the existing extra key check is now only used for backwards scans. Like the existing check, the new check won't always work out, but that seems like an acceptable price to pay. The new approach is more effective than just checking non-pivot tuples, especially with composite indexes and non-unique indexes. The high key represents an upper bound on all values that can appear on the page, which is often greater than whatever tuple happens to appear last at the time of the check. Also, suffix truncation's new logic for picking a split point will often result in high keys that are relatively dissimilar to the other (non-pivot) tuples on the page, and therefore more likely to indicate that the scan need not proceed to the next page. Note that even pre-pg_upgrade'd v3 indexes make use of this optimization. (This is Heikki's refactored version) --- src/backend/access/nbtree/nbtsearch.c | 86 ++++++++++++++++++--- src/backend/access/nbtree/nbtutils.c | 103 +++++++++++--------------- src/include/access/nbtree.h | 3 +- 3 files changed, 122 insertions(+), 70 deletions(-) diff --git a/src/backend/access/nbtree/nbtsearch.c b/src/backend/access/nbtree/nbtsearch.c index af3da3aa5b6..243be6f410d 100644 --- a/src/backend/access/nbtree/nbtsearch.c +++ b/src/backend/access/nbtree/nbtsearch.c @@ -1220,7 +1220,6 @@ _bt_readpage(IndexScanDesc scan, ScanDirection dir, OffsetNumber offnum) OffsetNumber minoff; OffsetNumber maxoff; int itemIndex; - IndexTuple itup; bool continuescan; /* @@ -1241,6 +1240,7 @@ _bt_readpage(IndexScanDesc scan, ScanDirection dir, OffsetNumber offnum) _bt_parallel_release(scan, BufferGetBlockNumber(so->currPos.buf)); } + continuescan = true; /* default assumption */ minoff = P_FIRSTDATAKEY(opaque); maxoff = PageGetMaxOffsetNumber(page); @@ -1282,23 +1282,58 @@ _bt_readpage(IndexScanDesc scan, ScanDirection dir, OffsetNumber offnum) while (offnum <= maxoff) { - itup = _bt_checkkeys(scan, page, offnum, dir, &continuescan); - if (itup != NULL) + ItemId iid = PageGetItemId(page, offnum); + IndexTuple itup; + + /* + * If the scan specifies not to return killed tuples, then we + * treat a killed tuple as not passing the qual. Most of the + * time, it's a win to not bother examining the tuple's index + * keys, but just skip to the next tuple. + */ + if (scan->ignore_killed_tuples && ItemIdIsDead(iid)) + { + offnum = OffsetNumberNext(offnum); + continue; + } + + itup = (IndexTuple) PageGetItem(page, iid); + + if (_bt_checkkeys(scan, itup, dir, &continuescan)) { /* tuple passes all scan key conditions, so remember it */ _bt_saveitem(so, itemIndex, offnum, itup); itemIndex++; } + /* When !continuescan, there can't be any more matches, so stop */ if (!continuescan) - { - /* there can't be any more matches, so stop */ - so->currPos.moreRight = false; break; - } offnum = OffsetNumberNext(offnum); } + /* + * We don't need to visit page to the right when the high key + * indicates that no more matches will be found there. + * + * Checking the high key like this works out more often than you might + * think. Leaf page splits pick a split point between the two most + * dissimilar tuples (this is weighed against the need to evenly share + * free space). Leaf pages with high key attribute values that can + * only appear on non-pivot tuples on the right sibling page are + * common. + */ + if (continuescan && !P_RIGHTMOST(opaque)) + { + ItemId iid = PageGetItemId(page, P_HIKEY); + IndexTuple itup = (IndexTuple) PageGetItem(page, iid); + + _bt_checkkeys(scan, itup, dir, &continuescan); + } + + if (!continuescan) + so->currPos.moreRight = false; + Assert(itemIndex <= MaxIndexTuplesPerPage); so->currPos.firstItem = 0; so->currPos.lastItem = itemIndex - 1; @@ -1313,8 +1348,41 @@ _bt_readpage(IndexScanDesc scan, ScanDirection dir, OffsetNumber offnum) while (offnum >= minoff) { - itup = _bt_checkkeys(scan, page, offnum, dir, &continuescan); - if (itup != NULL) + ItemId iid = PageGetItemId(page, offnum); + IndexTuple itup; + bool tuple_alive; + bool passes_quals; + + /* + * If the scan specifies not to return killed tuples, then we + * treat a killed tuple as not passing the qual. Most of the + * time, it's a win to not bother examining the tuple's index + * keys, but just skip to the next tuple (previous, actually, + * since we're scaning backwards) . However, if this is the + * first tuple on the page, we do check the index keys, to prevent + * uselessly advancing to the page to the left. This is similar + * to the high key optimization used by forward scans. + */ + if (scan->ignore_killed_tuples && ItemIdIsDead(iid)) + { + BTPageOpaque opaque = (BTPageOpaque) PageGetSpecialPointer(page); + + Assert(offnum >= P_FIRSTDATAKEY(opaque)); + if (offnum > P_FIRSTDATAKEY(opaque)) + { + offnum = OffsetNumberPrev(offnum); + continue; + } + + tuple_alive = false; + } + else + tuple_alive = true; + + itup = (IndexTuple) PageGetItem(page, iid); + + passes_quals = _bt_checkkeys(scan, itup, dir, &continuescan); + if (passes_quals && tuple_alive) { /* tuple passes all scan key conditions, so remember it */ itemIndex--; diff --git a/src/backend/access/nbtree/nbtutils.c b/src/backend/access/nbtree/nbtutils.c index 2c05fb5e451..1b1d889bba8 100644 --- a/src/backend/access/nbtree/nbtutils.c +++ b/src/backend/access/nbtree/nbtutils.c @@ -47,7 +47,7 @@ static bool _bt_compare_scankey_args(IndexScanDesc scan, ScanKey op, static bool _bt_fix_scankey_strategy(ScanKey skey, int16 *indoption); static void _bt_mark_scankey_required(ScanKey skey); static bool _bt_check_rowcompare(ScanKey skey, - IndexTuple tuple, TupleDesc tupdesc, + IndexTuple tuple, int tupnatts, TupleDesc tupdesc, ScanDirection dir, bool *continuescan); @@ -1350,8 +1350,7 @@ _bt_mark_scankey_required(ScanKey skey) /* * Test whether an indextuple satisfies all the scankey conditions. * - * If so, return the address of the index tuple on the index page. - * If not, return NULL. + * Returns true, if so. * * If the tuple fails to pass the qual, we also determine whether there's * any need to continue the scan beyond this tuple, and set *continuescan @@ -1359,21 +1358,19 @@ _bt_mark_scankey_required(ScanKey skey) * this is done. * * scan: index scan descriptor (containing a search-type scankey) - * page: buffer page containing index tuple - * offnum: offset number of index tuple (must be a valid item!) + * tuple: index tuple to test * dir: direction we are scanning in * continuescan: output parameter (will be set correctly in all cases) * - * Caller must hold pin and lock on the index page. + * Caller can pass a high key tuple in the hopes of discovering that the scan + * need not continue on to a page to the right. We don't currently bother + * limiting high key comparisons to SK_BT_REQFWD scan keys. */ -IndexTuple -_bt_checkkeys(IndexScanDesc scan, - Page page, OffsetNumber offnum, +bool +_bt_checkkeys(IndexScanDesc scan, IndexTuple tuple, ScanDirection dir, bool *continuescan) { - ItemId iid = PageGetItemId(page, offnum); - bool tuple_alive; - IndexTuple tuple; + int tupnatts; TupleDesc tupdesc; BTScanOpaque so; int keysz; @@ -1382,40 +1379,7 @@ _bt_checkkeys(IndexScanDesc scan, *continuescan = true; /* default assumption */ - /* - * If the scan specifies not to return killed tuples, then we treat a - * killed tuple as not passing the qual. Most of the time, it's a win to - * not bother examining the tuple's index keys, but just return - * immediately with continuescan = true to proceed to the next tuple. - * However, if this is the last tuple on the page, we should check the - * index keys to prevent uselessly advancing to the next page. - */ - if (scan->ignore_killed_tuples && ItemIdIsDead(iid)) - { - /* return immediately if there are more tuples on the page */ - if (ScanDirectionIsForward(dir)) - { - if (offnum < PageGetMaxOffsetNumber(page)) - return NULL; - } - else - { - BTPageOpaque opaque = (BTPageOpaque) PageGetSpecialPointer(page); - - if (offnum > P_FIRSTDATAKEY(opaque)) - return NULL; - } - - /* - * OK, we want to check the keys so we can set continuescan correctly, - * but we'll return NULL even if the tuple passes the key tests. - */ - tuple_alive = false; - } - else - tuple_alive = true; - - tuple = (IndexTuple) PageGetItem(page, iid); + tupnatts = BTreeTupleGetNAtts(tuple, scan->indexRelation); tupdesc = RelationGetDescr(scan->indexRelation); so = (BTScanOpaque) scan->opaque; @@ -1427,13 +1391,25 @@ _bt_checkkeys(IndexScanDesc scan, bool isNull; Datum test; - Assert(key->sk_attno <= BTreeTupleGetNAtts(tuple, scan->indexRelation)); + /* + * Assume that truncated attribute (from high key) passes the qual. + * The value of a truncated attribute for the first tuple on the right + * page could be any possible value, so we may have to visit the next + * page. + */ + if (key->sk_attno > tupnatts) + { + Assert(ScanDirectionIsForward(dir)); + continue; + } + /* row-comparison keys need special processing */ if (key->sk_flags & SK_ROW_HEADER) { - if (_bt_check_rowcompare(key, tuple, tupdesc, dir, continuescan)) + if (_bt_check_rowcompare(key, tuple, tupnatts, tupdesc, dir, + continuescan)) continue; - return NULL; + return false; } datum = index_getattr(tuple, @@ -1471,7 +1447,7 @@ _bt_checkkeys(IndexScanDesc scan, /* * In any case, this indextuple doesn't match the qual. */ - return NULL; + return false; } if (isNull) @@ -1512,7 +1488,7 @@ _bt_checkkeys(IndexScanDesc scan, /* * In any case, this indextuple doesn't match the qual. */ - return NULL; + return false; } test = FunctionCall2Coll(&key->sk_func, key->sk_collation, @@ -1540,16 +1516,12 @@ _bt_checkkeys(IndexScanDesc scan, /* * In any case, this indextuple doesn't match the qual. */ - return NULL; + return false; } } - /* Check for failure due to it being a killed tuple. */ - if (!tuple_alive) - return NULL; - /* If we get here, the tuple passes all index quals. */ - return tuple; + return true; } /* @@ -1562,8 +1534,8 @@ _bt_checkkeys(IndexScanDesc scan, * This is a subroutine for _bt_checkkeys, which see for more info. */ static bool -_bt_check_rowcompare(ScanKey skey, IndexTuple tuple, TupleDesc tupdesc, - ScanDirection dir, bool *continuescan) +_bt_check_rowcompare(ScanKey skey, IndexTuple tuple, int tupnatts, + TupleDesc tupdesc, ScanDirection dir, bool *continuescan) { ScanKey subkey = (ScanKey) DatumGetPointer(skey->sk_argument); int32 cmpresult = 0; @@ -1580,6 +1552,19 @@ _bt_check_rowcompare(ScanKey skey, IndexTuple tuple, TupleDesc tupdesc, Assert(subkey->sk_flags & SK_ROW_MEMBER); + /* + * Assume that truncated attribute (from high key) passes the qual. + * The value of a truncated attribute for the first tuple on the right + * page could be any possible value, so we may have to visit the next + * page. + */ + if (subkey->sk_attno > tupnatts) + { + Assert(ScanDirectionIsForward(dir)); + cmpresult = 0; + continue; + } + datum = index_getattr(tuple, subkey->sk_attno, tupdesc, diff --git a/src/include/access/nbtree.h b/src/include/access/nbtree.h index 60622ea7906..53e4e6d194d 100644 --- a/src/include/access/nbtree.h +++ b/src/include/access/nbtree.h @@ -586,8 +586,7 @@ extern bool _bt_advance_array_keys(IndexScanDesc scan, ScanDirection dir); extern void _bt_mark_array_keys(IndexScanDesc scan); extern void _bt_restore_array_keys(IndexScanDesc scan); extern void _bt_preprocess_keys(IndexScanDesc scan); -extern IndexTuple _bt_checkkeys(IndexScanDesc scan, - Page page, OffsetNumber offnum, +extern bool _bt_checkkeys(IndexScanDesc scan, IndexTuple tuple, ScanDirection dir, bool *continuescan); extern void _bt_killitems(IndexScanDesc scan); extern BTCycleId _bt_vacuum_cycleid(Relation rel); -- 2.20.1 --------------6F14E75AB34E6038C5D9183C-- ^ permalink raw reply [nested|flat] 32+ messages in thread
* [PATCH v19 2/8] Row pattern recognition patch (parse/analysis). @ 2024-05-14 23:26 Tatsuo Ishii <[email protected]> 0 siblings, 0 replies; 32+ messages in thread From: Tatsuo Ishii @ 2024-05-14 23:26 UTC (permalink / raw) --- src/backend/parser/parse_agg.c | 7 + src/backend/parser/parse_clause.c | 296 +++++++++++++++++++++++++++++- src/backend/parser/parse_expr.c | 6 + src/backend/parser/parse_func.c | 3 + 4 files changed, 311 insertions(+), 1 deletion(-) diff --git a/src/backend/parser/parse_agg.c b/src/backend/parser/parse_agg.c index bee7d8346a..9bc22a836a 100644 --- a/src/backend/parser/parse_agg.c +++ b/src/backend/parser/parse_agg.c @@ -577,6 +577,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 @@ -967,6 +971,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 8118036495..9762dce81f 100644 --- a/src/backend/parser/parse_clause.c +++ b/src/backend/parser/parse_clause.c @@ -98,7 +98,14 @@ 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 - @@ -2956,6 +2963,10 @@ transformWindowDefinitions(ParseState *pstate, rangeopfamily, rangeopcintype, &wc->endInRangeFunc, windef->endOffset); + + /* Process Row Pattern Recognition related clauses */ + transformRPR(pstate, wc, windef, targetlist); + wc->winref = winref; result = lappend(result, wc); @@ -3820,3 +3831,286 @@ 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 AFTER MACH SKIP TO variable */ + wc->rpSkipVariable = windef->rpCommonSyntax->rpSkipVariable; + + /* 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 aba3546ed1..e98b45e06e 100644 --- a/src/backend/parser/parse_expr.c +++ b/src/backend/parser/parse_expr.c @@ -578,6 +578,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; @@ -1860,6 +1861,9 @@ transformSubLink(ParseState *pstate, SubLink *sublink) case EXPR_KIND_GENERATED_COLUMN: err = _("cannot use subquery in column generation expression"); break; + case EXPR_KIND_RPR_DEFINE: + err = _("cannot use subquery in DEFINE expression"); + break; /* * There is intentionally no default: case here, so that the @@ -3197,6 +3201,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 9b23344a3b..4c482abb30 100644 --- a/src/backend/parser/parse_func.c +++ b/src/backend/parser/parse_func.c @@ -2658,6 +2658,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_May_15_09_02_03_2024_008)-- Content-Type: Text/X-Patch; charset=us-ascii Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="v19-0003-Row-pattern-recognition-patch-rewriter.patch" ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: per backend I/O statistics @ 2024-09-13 16:09 Bertrand Drouvot <[email protected]> 0 siblings, 1 reply; 32+ messages in thread From: Bertrand Drouvot @ 2024-09-13 16:09 UTC (permalink / raw) To: Nazir Bilal Yavuz <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected] Hi, On Fri, Sep 13, 2024 at 04:45:08PM +0300, Nazir Bilal Yavuz wrote: > Hi, > > Thanks for working on this! > > Your patch applies and builds cleanly. Thanks for looking at it! > On Fri, 6 Sept 2024 at 18:03, Bertrand Drouvot > <[email protected]> wrote: > > > > - As stated up-thread, the pg_stat_get_backend_io() behaves as if > > stats_fetch_consistency is set to none (each execution re-fetches counters > > from shared memory). Indeed, the snapshot is not build in each backend to copy > > all the others backends stats, as 1/ there is no use case (there is no need to > > get others backends I/O statistics while taking care of the stats_fetch_consistency) > > and 2/ that could be memory expensive depending of the number of max connections. > > I believe this is the correct approach. Thanks for sharing your thoughts. > I manually tested your patches, and they work as expected. Here is > some feedback: > > - The stats_reset column is NULL in both pg_my_stat_io and > pg_stat_get_backend_io() until the first call to reset io statistics. > While I'm not sure if it's necessary, it might be useful to display > the more recent of the two times in the stats_reset column: the > statistics reset time or the backend creation time. I'm not sure about that as one can already get the backend "creation time" through pg_stat_activity.backend_start. > - The pgstat_reset_io_counter_internal() is called in the > pgstat_shutdown_hook(). This causes the stats_reset column showing the > termination time of the old backend when its proc num is reassigned to > a new backend. doh! Nice catch, thanks! And also new backends that are not re-using a previous "existing" process slot are getting the last reset time (if any). So the right place to fix this is in pgstat_io_init_backend_cb(): done in v4 attached. v4 also sets the reset time to 0 in pgstat_shutdown_hook() (but that's not necessary though, that's just to be right "conceptually" speaking). Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com Attachments: [text/x-diff] v4-0001-per-backend-I-O-statistics.patch (30.6K, ../../[email protected]/2-v4-0001-per-backend-I-O-statistics.patch) download | inline diff: From 2bc490060ce8ed6557ca5a586f36cd43e7480d8e Mon Sep 17 00:00:00 2001 From: Bertrand Drouvot <[email protected]> Date: Thu, 22 Aug 2024 15:16:50 +0000 Subject: [PATCH v4 1/2] per backend I/O statistics While pg_stat_io provides cluster-wide I/O statistics, this commit adds a new pg_my_stat_io view to display "my" backend I/O statistics. The KIND_IO stats are still "fixed amount" ones as the maximum number of backend is fixed. The statistics snapshot is made for the global stats (the aggregated ones) and for my backend stats. The snapshot is not build in each backend to copy all the others backends stats, as 1/ there is no use case (there is no need to get others backends I/O statistics while taking care of the stats_fetch_consistency) and 2/ that could be memory expensive depending of the number of max connections. A subsequent commit will add a new pg_stat_get_backend_io() function to be able to retrieve the I/O statistics for a given backend pid (each execution re-fetches counters from shared memory because as stated above there is no snapshots being created in each backend to copy the other backends stats). XXX: Bump catalog version needs to be done. --- doc/src/sgml/config.sgml | 4 +- doc/src/sgml/monitoring.sgml | 28 ++++++ src/backend/catalog/system_views.sql | 22 +++++ src/backend/utils/activity/pgstat.c | 11 ++- src/backend/utils/activity/pgstat_io.c | 93 +++++++++++++++--- src/backend/utils/activity/pgstat_shmem.c | 4 +- src/backend/utils/adt/pgstatfuncs.c | 114 +++++++++++++++++++++- src/include/catalog/pg_proc.dat | 9 ++ src/include/pgstat.h | 14 ++- src/include/utils/pgstat_internal.h | 14 ++- src/test/regress/expected/rules.out | 19 ++++ src/test/regress/expected/stats.out | 44 ++++++++- src/test/regress/sql/stats.sql | 25 ++++- 13 files changed, 368 insertions(+), 33 deletions(-) 8.4% doc/src/sgml/ 29.9% src/backend/utils/activity/ 23.7% src/backend/utils/adt/ 4.4% src/include/catalog/ 3.1% src/include/utils/ 3.1% src/include/ 14.3% src/test/regress/expected/ 9.9% src/test/regress/sql/ diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 0aec11f443..2a59b97093 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -8331,7 +8331,9 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv; displayed in <link linkend="monitoring-pg-stat-database-view"> <structname>pg_stat_database</structname></link>, <link linkend="monitoring-pg-stat-io-view"> - <structname>pg_stat_io</structname></link>, in the output of + <structname>pg_stat_io</structname></link>, + <link linkend="monitoring-pg-my-stat-io-view"> + <structname>pg_my_stat_io</structname></link>, in the output of <xref linkend="sql-explain"/> when the <literal>BUFFERS</literal> option is used, in the output of <xref linkend="sql-vacuum"/> when the <literal>VERBOSE</literal> option is used, by autovacuum diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml index 933de6fe07..b28ca4e0ca 100644 --- a/doc/src/sgml/monitoring.sgml +++ b/doc/src/sgml/monitoring.sgml @@ -488,6 +488,16 @@ postgres 27093 0.0 0.0 30096 2752 ? Ss 11:34 0:00 postgres: ser </entry> </row> + <row> + <entry><structname>pg_my_stat_io</structname><indexterm><primary>pg_my_stat_io</primary></indexterm></entry> + <entry> + One row for each combination of context and target object containing + my backend I/O statistics. + See <link linkend="monitoring-pg-my-stat-io-view"> + <structname>pg_my_stat_io</structname></link> for details. + </entry> + </row> + <row> <entry><structname>pg_stat_replication_slots</structname><indexterm><primary>pg_stat_replication_slots</primary></indexterm></entry> <entry>One row per replication slot, showing statistics about the @@ -2948,6 +2958,24 @@ description | Waiting for a newly initialized WAL file to reach durable storage + </sect2> + + <sect2 id="monitoring-pg-my-stat-io-view"> + <title><structname>pg_my_stat_io</structname></title> + + <indexterm> + <primary>pg_my_stat_io</primary> + </indexterm> + + <para> + The <structname>pg_my_stat_io</structname> view will contain one row for each + combination of target I/O object and I/O context, showing + my backend I/O statistics. Combinations which do not make sense are + omitted. The fields are exactly the same as the ones in the <link + linkend="monitoring-pg-stat-io-view"> <structname>pg_stat_io</structname></link> + view. + </para> + </sect2> <sect2 id="monitoring-pg-stat-bgwriter-view"> diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql index 7fd5d256a1..b4939b7bc6 100644 --- a/src/backend/catalog/system_views.sql +++ b/src/backend/catalog/system_views.sql @@ -1168,6 +1168,28 @@ SELECT b.stats_reset FROM pg_stat_get_io() b; +CREATE VIEW pg_my_stat_io AS +SELECT + b.backend_type, + b.object, + b.context, + b.reads, + b.read_time, + b.writes, + b.write_time, + b.writebacks, + b.writeback_time, + b.extends, + b.extend_time, + b.op_bytes, + b.hits, + b.evictions, + b.reuses, + b.fsyncs, + b.fsync_time, + b.stats_reset +FROM pg_stat_get_my_io() b; + CREATE VIEW pg_stat_wal AS SELECT w.wal_records, diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c index a7f2dfc744..fda4ca9a4a 100644 --- a/src/backend/utils/activity/pgstat.c +++ b/src/backend/utils/activity/pgstat.c @@ -406,10 +406,12 @@ static const PgStat_KindInfo pgstat_kind_builtin_infos[PGSTAT_KIND_BUILTIN_SIZE] .fixed_amount = true, - .snapshot_ctl_off = offsetof(PgStat_Snapshot, io), + .init_backend_cb = pgstat_io_init_backend_cb, + .snapshot_ctl_off = offsetof(PgStat_Snapshot, global_io), .shared_ctl_off = offsetof(PgStat_ShmemControl, io), - .shared_data_off = offsetof(PgStatShared_IO, stats), - .shared_data_len = sizeof(((PgStatShared_IO *) 0)->stats), + /* [de-]serialize global_stats only */ + .shared_data_off = offsetof(PgStatShared_IO, global_stats), + .shared_data_len = sizeof(((PgStatShared_IO *) 0)->global_stats), .flush_fixed_cb = pgstat_io_flush_cb, .have_fixed_pending_cb = pgstat_io_have_pending_cb, @@ -587,6 +589,9 @@ pgstat_shutdown_hook(int code, Datum arg) pgstat_report_stat(true); + /* reset the pgstat_io counter related to this proc number */ + pgstat_reset_io_counter_internal(MyProcNumber, 0); + /* there shouldn't be any pending changes left */ Assert(dlist_is_empty(&pgStatPending)); dlist_init(&pgStatPending); diff --git a/src/backend/utils/activity/pgstat_io.c b/src/backend/utils/activity/pgstat_io.c index cc2ffc78aa..d32121fc1f 100644 --- a/src/backend/utils/activity/pgstat_io.c +++ b/src/backend/utils/activity/pgstat_io.c @@ -31,6 +31,7 @@ typedef struct PgStat_PendingIO static PgStat_PendingIO PendingIOStats; static bool have_iostats = false; +void pgstat_reset_io_counter_internal(int index, TimestampTz ts); /* * Check that stats have not been counted for any combination of IOObject, @@ -154,11 +155,19 @@ pgstat_count_io_op_time(IOObject io_object, IOContext io_context, IOOp io_op, } PgStat_IO * -pgstat_fetch_stat_io(void) +pgstat_fetch_global_stat_io(void) { pgstat_snapshot_fixed(PGSTAT_KIND_IO); - return &pgStatLocal.snapshot.io; + return &pgStatLocal.snapshot.global_io; +} + +PgStat_Backend_IO * +pgstat_fetch_my_stat_io(void) +{ + pgstat_snapshot_fixed(PGSTAT_KIND_IO); + + return &pgStatLocal.snapshot.my_io; } /* @@ -192,13 +201,16 @@ pgstat_io_flush_cb(bool nowait) { LWLock *bktype_lock; PgStat_BktypeIO *bktype_shstats; + PgStat_BktypeIO *global_bktype_shstats; if (!have_iostats) return false; bktype_lock = &pgStatLocal.shmem->io.locks[MyBackendType]; bktype_shstats = - &pgStatLocal.shmem->io.stats.stats[MyBackendType]; + &pgStatLocal.shmem->io.stat[MyProcNumber].stats; + global_bktype_shstats = + &pgStatLocal.shmem->io.global_stats.stats[MyBackendType]; if (!nowait) LWLockAcquire(bktype_lock, LW_EXCLUSIVE); @@ -212,19 +224,28 @@ pgstat_io_flush_cb(bool nowait) for (int io_op = 0; io_op < IOOP_NUM_TYPES; io_op++) { instr_time time; + PgStat_Counter counter; + + counter = PendingIOStats.counts[io_object][io_context][io_op]; - bktype_shstats->counts[io_object][io_context][io_op] += - PendingIOStats.counts[io_object][io_context][io_op]; + bktype_shstats->counts[io_object][io_context][io_op] += counter; + + global_bktype_shstats->counts[io_object][io_context][io_op] += + counter; time = PendingIOStats.pending_times[io_object][io_context][io_op]; bktype_shstats->times[io_object][io_context][io_op] += INSTR_TIME_GET_MICROSEC(time); + + global_bktype_shstats->times[io_object][io_context][io_op] += + INSTR_TIME_GET_MICROSEC(time); } } } Assert(pgstat_bktype_io_stats_valid(bktype_shstats, MyBackendType)); + Assert(pgstat_bktype_io_stats_valid(global_bktype_shstats, MyBackendType)); LWLockRelease(bktype_lock); @@ -278,13 +299,35 @@ pgstat_io_init_shmem_cb(void *stats) LWLockInitialize(&stat_shmem->locks[i], LWTRANCHE_PGSTATS_DATA); } +void +pgstat_reset_io_counter_internal(int index, TimestampTz ts) +{ + LWLock *bktype_lock = &pgStatLocal.shmem->io.locks[0]; + PgStat_BktypeIO *bktype_shstats = &pgStatLocal.shmem->io.stat[index].stats; + + + /* + * Use the lock in the first BackendType's PgStat_BktypeIO to protect the + * reset timestamp as well. + */ + LWLockAcquire(bktype_lock, LW_EXCLUSIVE); + + pgStatLocal.shmem->io.stat[index].stat_reset_timestamp = ts; + + memset(bktype_shstats, 0, sizeof(*bktype_shstats)); + LWLockRelease(bktype_lock); +} + void pgstat_io_reset_all_cb(TimestampTz ts) { + for (int i = 0; i < NumProcStatSlots; i++) + pgstat_reset_io_counter_internal(i, ts); + for (int i = 0; i < BACKEND_NUM_TYPES; i++) { LWLock *bktype_lock = &pgStatLocal.shmem->io.locks[i]; - PgStat_BktypeIO *bktype_shstats = &pgStatLocal.shmem->io.stats.stats[i]; + PgStat_BktypeIO *bktype_shstats = &pgStatLocal.shmem->io.global_stats.stats[i]; LWLockAcquire(bktype_lock, LW_EXCLUSIVE); @@ -293,7 +336,7 @@ pgstat_io_reset_all_cb(TimestampTz ts) * the reset timestamp as well. */ if (i == 0) - pgStatLocal.shmem->io.stats.stat_reset_timestamp = ts; + pgStatLocal.shmem->io.global_stats.stat_reset_timestamp = ts; memset(bktype_shstats, 0, sizeof(*bktype_shstats)); LWLockRelease(bktype_lock); @@ -303,11 +346,28 @@ pgstat_io_reset_all_cb(TimestampTz ts) void pgstat_io_snapshot_cb(void) { + /* First, our own stats */ + PgStat_BktypeIO *bktype_shstats = &pgStatLocal.shmem->io.stat[MyProcNumber].stats; + PgStat_BktypeIO *bktype_snap = &pgStatLocal.snapshot.my_io.stats; + LWLock *mybktype_lock = &pgStatLocal.shmem->io.locks[0]; + + LWLockAcquire(mybktype_lock, LW_SHARED); + + pgStatLocal.snapshot.my_io.stat_reset_timestamp = + pgStatLocal.shmem->io.stat[MyProcNumber].stat_reset_timestamp; + + pgStatLocal.snapshot.my_io.bktype = + pgStatLocal.shmem->io.stat[MyProcNumber].bktype; + + *bktype_snap = *bktype_shstats; + LWLockRelease(mybktype_lock); + + /* Now, the global stats */ for (int i = 0; i < BACKEND_NUM_TYPES; i++) { LWLock *bktype_lock = &pgStatLocal.shmem->io.locks[i]; - PgStat_BktypeIO *bktype_shstats = &pgStatLocal.shmem->io.stats.stats[i]; - PgStat_BktypeIO *bktype_snap = &pgStatLocal.snapshot.io.stats[i]; + PgStat_BktypeIO *bktype_global_shstats = &pgStatLocal.shmem->io.global_stats.stats[i]; + PgStat_BktypeIO *bktype_global_snap = &pgStatLocal.snapshot.global_io.stats[i]; LWLockAcquire(bktype_lock, LW_SHARED); @@ -316,11 +376,11 @@ pgstat_io_snapshot_cb(void) * the reset timestamp as well. */ if (i == 0) - pgStatLocal.snapshot.io.stat_reset_timestamp = - pgStatLocal.shmem->io.stats.stat_reset_timestamp; + pgStatLocal.snapshot.global_io.stat_reset_timestamp = + pgStatLocal.shmem->io.global_stats.stat_reset_timestamp; /* using struct assignment due to better type safety */ - *bktype_snap = *bktype_shstats; + *bktype_global_snap = *bktype_global_shstats; LWLockRelease(bktype_lock); } } @@ -503,3 +563,12 @@ pgstat_tracks_io_op(BackendType bktype, IOObject io_object, return true; } + +void +pgstat_io_init_backend_cb(void) +{ + /* Initialize our backend type in the IO statistics */ + pgStatLocal.shmem->io.stat[MyProcNumber].bktype = MyBackendType; + /* Set the stat_reset_timestamp to zero */ + pgStatLocal.shmem->io.stat[MyProcNumber].stat_reset_timestamp = 0; +} diff --git a/src/backend/utils/activity/pgstat_shmem.c b/src/backend/utils/activity/pgstat_shmem.c index ec93bf6902..3cfcba9609 100644 --- a/src/backend/utils/activity/pgstat_shmem.c +++ b/src/backend/utils/activity/pgstat_shmem.c @@ -128,7 +128,7 @@ StatsShmemSize(void) { Size sz; - sz = MAXALIGN(sizeof(PgStat_ShmemControl)); + sz = MAXALIGN(sizeof(PgStat_ShmemControl) + mul_size(sizeof(PgStat_Backend_IO), NumProcStatSlots)); sz = add_size(sz, pgstat_dsa_init_size()); /* Add shared memory for all the custom fixed-numbered statistics */ @@ -172,7 +172,7 @@ StatsShmemInit(void) Assert(!found); /* the allocation of pgStatLocal.shmem itself */ - p += MAXALIGN(sizeof(PgStat_ShmemControl)); + p += MAXALIGN(sizeof(PgStat_ShmemControl) + mul_size(sizeof(PgStat_Backend_IO), NumProcStatSlots)); /* * Create a small dsa allocation in plain shared memory. This is diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c index 33c7b25560..4c00570396 100644 --- a/src/backend/utils/adt/pgstatfuncs.c +++ b/src/backend/utils/adt/pgstatfuncs.c @@ -1363,14 +1363,17 @@ pg_stat_get_io(PG_FUNCTION_ARGS) InitMaterializedSRF(fcinfo, 0); rsinfo = (ReturnSetInfo *) fcinfo->resultinfo; - backends_io_stats = pgstat_fetch_stat_io(); + backends_io_stats = pgstat_fetch_global_stat_io(); reset_time = TimestampTzGetDatum(backends_io_stats->stat_reset_timestamp); for (int bktype = 0; bktype < BACKEND_NUM_TYPES; bktype++) { - Datum bktype_desc = CStringGetTextDatum(GetBackendTypeDesc(bktype)); - PgStat_BktypeIO *bktype_stats = &backends_io_stats->stats[bktype]; + Datum bktype_desc; + PgStat_BktypeIO *bktype_stats; + + bktype_desc = CStringGetTextDatum(GetBackendTypeDesc(bktype)); + bktype_stats = &backends_io_stats->stats[bktype]; /* * In Assert builds, we can afford an extra loop through all of the @@ -1462,6 +1465,111 @@ pg_stat_get_io(PG_FUNCTION_ARGS) return (Datum) 0; } +Datum +pg_stat_get_my_io(PG_FUNCTION_ARGS) +{ + ReturnSetInfo *rsinfo; + PgStat_Backend_IO *backend_io_stats; + Datum bktype_desc; + PgStat_BktypeIO *bktype_stats; + BackendType bktype; + Datum reset_time; + + InitMaterializedSRF(fcinfo, 0); + rsinfo = (ReturnSetInfo *) fcinfo->resultinfo; + + backend_io_stats = pgstat_fetch_my_stat_io(); + + bktype = backend_io_stats->bktype; + bktype_desc = CStringGetTextDatum(GetBackendTypeDesc(bktype)); + bktype_stats = &backend_io_stats->stats; + reset_time = TimestampTzGetDatum(backend_io_stats->stat_reset_timestamp); + + /* + * In Assert builds, we can afford an extra loop through all of the + * counters checking that only expected stats are non-zero, since it keeps + * the non-Assert code cleaner. + */ + Assert(pgstat_bktype_io_stats_valid(bktype_stats, bktype)); + + for (int io_obj = 0; io_obj < IOOBJECT_NUM_TYPES; io_obj++) + { + const char *obj_name = pgstat_get_io_object_name(io_obj); + + for (int io_context = 0; io_context < IOCONTEXT_NUM_TYPES; io_context++) + { + const char *context_name = pgstat_get_io_context_name(io_context); + + Datum values[IO_NUM_COLUMNS] = {0}; + bool nulls[IO_NUM_COLUMNS] = {0}; + + /* + * Some combinations of BackendType, IOObject, and IOContext are + * not valid for any type of IOOp. In such cases, omit the entire + * row from the view. + */ + if (!pgstat_tracks_io_object(bktype, io_obj, io_context)) + continue; + + values[IO_COL_BACKEND_TYPE] = bktype_desc; + values[IO_COL_CONTEXT] = CStringGetTextDatum(context_name); + values[IO_COL_OBJECT] = CStringGetTextDatum(obj_name); + if (backend_io_stats->stat_reset_timestamp != 0) + values[IO_COL_RESET_TIME] = reset_time; + else + nulls[IO_COL_RESET_TIME] = true; + + /* + * Hard-code this to the value of BLCKSZ for now. Future values + * could include XLOG_BLCKSZ, once WAL IO is tracked, and constant + * multipliers, once non-block-oriented IO (e.g. temporary file + * IO) is tracked. + */ + values[IO_COL_CONVERSION] = Int64GetDatum(BLCKSZ); + + for (int io_op = 0; io_op < IOOP_NUM_TYPES; io_op++) + { + int op_idx = pgstat_get_io_op_index(io_op); + int time_idx = pgstat_get_io_time_index(io_op); + + /* + * Some combinations of BackendType and IOOp, of IOContext and + * IOOp, and of IOObject and IOOp are not tracked. Set these + * cells in the view NULL. + */ + if (pgstat_tracks_io_op(bktype, io_obj, io_context, io_op)) + { + PgStat_Counter count = + bktype_stats->counts[io_obj][io_context][io_op]; + + values[op_idx] = Int64GetDatum(count); + } + else + nulls[op_idx] = true; + + /* not every operation is timed */ + if (time_idx == IO_COL_INVALID) + continue; + + if (!nulls[op_idx]) + { + PgStat_Counter time = + bktype_stats->times[io_obj][io_context][io_op]; + + values[time_idx] = Float8GetDatum(pg_stat_us_to_ms(time)); + } + else + nulls[time_idx] = true; + } + + tuplestore_putvalues(rsinfo->setResult, rsinfo->setDesc, + values, nulls); + } + } + + return (Datum) 0; +} + /* * Returns statistics of WAL activity */ diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat index 53a081ed88..4d5af3d7cf 100644 --- a/src/include/catalog/pg_proc.dat +++ b/src/include/catalog/pg_proc.dat @@ -5880,6 +5880,15 @@ proargnames => '{backend_type,object,context,reads,read_time,writes,write_time,writebacks,writeback_time,extends,extend_time,op_bytes,hits,evictions,reuses,fsyncs,fsync_time,stats_reset}', prosrc => 'pg_stat_get_io' }, +{ oid => '8806', descr => 'statistics: My backend IO statistics', + proname => 'pg_stat_get_my_io', prorows => '5', proretset => 't', + provolatile => 'v', proparallel => 'r', prorettype => 'record', + proargtypes => '', + proallargtypes => '{text,text,text,int8,float8,int8,float8,int8,float8,int8,float8,int8,int8,int8,int8,int8,float8,timestamptz}', + proargmodes => '{o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o}', + proargnames => '{backend_type,object,context,reads,read_time,writes,write_time,writebacks,writeback_time,extends,extend_time,op_bytes,hits,evictions,reuses,fsyncs,fsync_time,stats_reset}', + prosrc => 'pg_stat_get_my_io' }, + { oid => '1136', descr => 'statistics: information about WAL activity', proname => 'pg_stat_get_wal', proisstrict => 'f', provolatile => 's', proparallel => 'r', prorettype => 'record', proargtypes => '', diff --git a/src/include/pgstat.h b/src/include/pgstat.h index be2c91168a..9d63b1a5b7 100644 --- a/src/include/pgstat.h +++ b/src/include/pgstat.h @@ -16,6 +16,7 @@ #include "portability/instr_time.h" #include "postmaster/pgarch.h" /* for MAX_XFN_CHARS */ #include "replication/conflict.h" +#include "storage/proc.h" #include "utils/backend_progress.h" /* for backward compatibility */ #include "utils/backend_status.h" /* for backward compatibility */ #include "utils/relcache.h" @@ -76,6 +77,8 @@ */ #define PGSTAT_KIND_EXPERIMENTAL 128 +#define NumProcStatSlots (MaxBackends + NUM_AUXILIARY_PROCS) + static inline bool pgstat_is_kind_builtin(PgStat_Kind kind) { @@ -351,6 +354,12 @@ typedef struct PgStat_IO PgStat_BktypeIO stats[BACKEND_NUM_TYPES]; } PgStat_IO; +typedef struct PgStat_Backend_IO +{ + TimestampTz stat_reset_timestamp; + BackendType bktype; + PgStat_BktypeIO stats; +} PgStat_Backend_IO; typedef struct PgStat_StatDBEntry { @@ -559,7 +568,8 @@ extern instr_time pgstat_prepare_io_time(bool track_io_guc); extern void pgstat_count_io_op_time(IOObject io_object, IOContext io_context, IOOp io_op, instr_time start_time, uint32 cnt); -extern PgStat_IO *pgstat_fetch_stat_io(void); +extern PgStat_IO *pgstat_fetch_global_stat_io(void); +extern PgStat_Backend_IO *pgstat_fetch_my_stat_io(void); extern const char *pgstat_get_io_context_name(IOContext io_context); extern const char *pgstat_get_io_object_name(IOObject io_object); @@ -568,7 +578,7 @@ extern bool pgstat_tracks_io_object(BackendType bktype, IOObject io_object, IOContext io_context); extern bool pgstat_tracks_io_op(BackendType bktype, IOObject io_object, IOContext io_context, IOOp io_op); - +extern void pgstat_reset_io_counter_internal(int index, TimestampTz ts); /* * Functions in pgstat_database.c diff --git a/src/include/utils/pgstat_internal.h b/src/include/utils/pgstat_internal.h index bba90e898d..ea4cc6a2c6 100644 --- a/src/include/utils/pgstat_internal.h +++ b/src/include/utils/pgstat_internal.h @@ -366,11 +366,12 @@ typedef struct PgStatShared_Checkpointer typedef struct PgStatShared_IO { /* - * locks[i] protects stats.stats[i]. locks[0] also protects - * stats.stat_reset_timestamp. + * locks[i] protects global_stats.stats[i]. locks[0] also protects + * global_stats.stat_reset_timestamp. */ LWLock locks[BACKEND_NUM_TYPES]; - PgStat_IO stats; + PgStat_IO global_stats; + PgStat_Backend_IO stat[FLEXIBLE_ARRAY_MEMBER]; } PgStatShared_IO; typedef struct PgStatShared_SLRU @@ -463,7 +464,6 @@ typedef struct PgStat_ShmemControl PgStatShared_Archiver archiver; PgStatShared_BgWriter bgwriter; PgStatShared_Checkpointer checkpointer; - PgStatShared_IO io; PgStatShared_SLRU slru; PgStatShared_Wal wal; @@ -473,6 +473,8 @@ typedef struct PgStat_ShmemControl */ void *custom_data[PGSTAT_KIND_CUSTOM_SIZE]; + /* has to be at the end due to FLEXIBLE_ARRAY_MEMBER */ + PgStatShared_IO io; } PgStat_ShmemControl; @@ -494,7 +496,8 @@ typedef struct PgStat_Snapshot PgStat_CheckpointerStats checkpointer; - PgStat_IO io; + PgStat_Backend_IO my_io; + PgStat_IO global_io; PgStat_SLRUStats slru[SLRU_NUM_ELEMENTS]; @@ -629,6 +632,7 @@ 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); +extern void pgstat_io_init_backend_cb(void); /* diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out index a1626f3fae..785298d7b0 100644 --- a/src/test/regress/expected/rules.out +++ b/src/test/regress/expected/rules.out @@ -1398,6 +1398,25 @@ pg_matviews| SELECT n.nspname AS schemaname, LEFT JOIN pg_namespace n ON ((n.oid = c.relnamespace))) LEFT JOIN pg_tablespace t ON ((t.oid = c.reltablespace))) WHERE (c.relkind = 'm'::"char"); +pg_my_stat_io| SELECT backend_type, + object, + context, + reads, + read_time, + writes, + write_time, + writebacks, + writeback_time, + extends, + extend_time, + op_bytes, + hits, + evictions, + reuses, + fsyncs, + fsync_time, + stats_reset + FROM pg_stat_get_my_io() b(backend_type, object, context, reads, read_time, writes, write_time, writebacks, writeback_time, extends, extend_time, op_bytes, hits, evictions, reuses, fsyncs, fsync_time, stats_reset); pg_policies| SELECT n.nspname AS schemaname, c.relname AS tablename, pol.polname AS policyname, diff --git a/src/test/regress/expected/stats.out b/src/test/regress/expected/stats.out index 6e08898b18..c489e528e0 100644 --- a/src/test/regress/expected/stats.out +++ b/src/test/regress/expected/stats.out @@ -1249,7 +1249,7 @@ SELECT pg_stat_get_subscription_stats(NULL); (1 row) --- Test that the following operations are tracked in pg_stat_io: +-- Test that the following operations are tracked in pg_[my]_stat_io: -- - reads of target blocks into shared buffers -- - writes of shared buffers to permanent storage -- - extends of relations using shared buffers @@ -1261,9 +1261,14 @@ SELECT pg_stat_get_subscription_stats(NULL); -- extends. SELECT sum(extends) AS io_sum_shared_before_extends FROM pg_stat_io WHERE context = 'normal' AND object = 'relation' \gset +SELECT sum(extends) AS my_io_sum_shared_before_extends + FROM pg_my_stat_io WHERE context = 'normal' AND object = 'relation' \gset SELECT sum(writes) AS writes, sum(fsyncs) AS fsyncs FROM pg_stat_io WHERE object = 'relation' \gset io_sum_shared_before_ +SELECT sum(writes) AS writes, sum(fsyncs) AS fsyncs + FROM pg_my_stat_io + WHERE object = 'relation' \gset my_io_sum_shared_before_ CREATE TABLE test_io_shared(a int); INSERT INTO test_io_shared SELECT i FROM generate_series(1,100)i; SELECT pg_stat_force_next_flush(); @@ -1280,8 +1285,16 @@ SELECT :io_sum_shared_after_extends > :io_sum_shared_before_extends; t (1 row) +SELECT sum(extends) AS my_io_sum_shared_after_extends + FROM pg_my_stat_io WHERE context = 'normal' AND object = 'relation' \gset +SELECT :my_io_sum_shared_after_extends > :my_io_sum_shared_before_extends; + ?column? +---------- + t +(1 row) + -- After a checkpoint, there should be some additional IOCONTEXT_NORMAL writes --- and fsyncs. +-- and fsyncs in the global stats (not for the backend). -- See comment above for rationale for two explicit CHECKPOINTs. CHECKPOINT; CHECKPOINT; @@ -1301,6 +1314,23 @@ SELECT current_setting('fsync') = 'off' t (1 row) +SELECT sum(writes) AS writes, sum(fsyncs) AS fsyncs + FROM pg_my_stat_io + WHERE object = 'relation' \gset my_io_sum_shared_after_ +SELECT :my_io_sum_shared_after_writes >= :my_io_sum_shared_before_writes; + ?column? +---------- + t +(1 row) + +SELECT current_setting('fsync') = 'off' + OR (:my_io_sum_shared_after_fsyncs = :my_io_sum_shared_before_fsyncs + AND :my_io_sum_shared_after_fsyncs= 0); + ?column? +---------- + t +(1 row) + -- Change the tablespace so that the table is rewritten directly, then SELECT -- from it to cause it to be read back into shared buffers. SELECT sum(reads) AS io_sum_shared_before_reads @@ -1521,6 +1551,8 @@ SELECT pg_stat_have_stats('io', 0, 0); SELECT sum(evictions) + sum(reuses) + sum(extends) + sum(fsyncs) + sum(reads) + sum(writes) + sum(writebacks) + sum(hits) AS io_stats_pre_reset FROM pg_stat_io \gset +SELECT sum(evictions) + sum(reuses) + sum(extends) + sum(fsyncs) + sum(reads) + sum(writes) + sum(writebacks) + sum(hits) AS my_io_stats_pre_reset + FROM pg_my_stat_io \gset SELECT pg_stat_reset_shared('io'); pg_stat_reset_shared ---------------------- @@ -1535,6 +1567,14 @@ SELECT :io_stats_post_reset < :io_stats_pre_reset; t (1 row) +SELECT sum(evictions) + sum(reuses) + sum(extends) + sum(fsyncs) + sum(reads) + sum(writes) + sum(writebacks) + sum(hits) AS my_io_stats_post_reset + FROM pg_my_stat_io \gset +SELECT :my_io_stats_post_reset < :my_io_stats_pre_reset; + ?column? +---------- + t +(1 row) + -- test BRIN index doesn't block HOT update CREATE TABLE brin_hot ( id integer PRIMARY KEY, diff --git a/src/test/regress/sql/stats.sql b/src/test/regress/sql/stats.sql index d8ac0d06f4..c95cb71652 100644 --- a/src/test/regress/sql/stats.sql +++ b/src/test/regress/sql/stats.sql @@ -595,7 +595,7 @@ SELECT pg_stat_get_replication_slot(NULL); SELECT pg_stat_get_subscription_stats(NULL); --- Test that the following operations are tracked in pg_stat_io: +-- Test that the following operations are tracked in pg_[my]_stat_io: -- - reads of target blocks into shared buffers -- - writes of shared buffers to permanent storage -- - extends of relations using shared buffers @@ -609,18 +609,26 @@ SELECT pg_stat_get_subscription_stats(NULL); -- extends. SELECT sum(extends) AS io_sum_shared_before_extends FROM pg_stat_io WHERE context = 'normal' AND object = 'relation' \gset +SELECT sum(extends) AS my_io_sum_shared_before_extends + FROM pg_my_stat_io WHERE context = 'normal' AND object = 'relation' \gset SELECT sum(writes) AS writes, sum(fsyncs) AS fsyncs FROM pg_stat_io WHERE object = 'relation' \gset io_sum_shared_before_ +SELECT sum(writes) AS writes, sum(fsyncs) AS fsyncs + FROM pg_my_stat_io + WHERE object = 'relation' \gset my_io_sum_shared_before_ CREATE TABLE test_io_shared(a int); INSERT INTO test_io_shared SELECT i FROM generate_series(1,100)i; SELECT pg_stat_force_next_flush(); SELECT sum(extends) AS io_sum_shared_after_extends FROM pg_stat_io WHERE context = 'normal' AND object = 'relation' \gset SELECT :io_sum_shared_after_extends > :io_sum_shared_before_extends; +SELECT sum(extends) AS my_io_sum_shared_after_extends + FROM pg_my_stat_io WHERE context = 'normal' AND object = 'relation' \gset +SELECT :my_io_sum_shared_after_extends > :my_io_sum_shared_before_extends; -- After a checkpoint, there should be some additional IOCONTEXT_NORMAL writes --- and fsyncs. +-- and fsyncs in the global stats (not for the backend). -- See comment above for rationale for two explicit CHECKPOINTs. CHECKPOINT; CHECKPOINT; @@ -630,7 +638,13 @@ SELECT sum(writes) AS writes, sum(fsyncs) AS fsyncs SELECT :io_sum_shared_after_writes > :io_sum_shared_before_writes; SELECT current_setting('fsync') = 'off' OR :io_sum_shared_after_fsyncs > :io_sum_shared_before_fsyncs; - +SELECT sum(writes) AS writes, sum(fsyncs) AS fsyncs + FROM pg_my_stat_io + WHERE object = 'relation' \gset my_io_sum_shared_after_ +SELECT :my_io_sum_shared_after_writes >= :my_io_sum_shared_before_writes; +SELECT current_setting('fsync') = 'off' + OR (:my_io_sum_shared_after_fsyncs = :my_io_sum_shared_before_fsyncs + AND :my_io_sum_shared_after_fsyncs= 0); -- Change the tablespace so that the table is rewritten directly, then SELECT -- from it to cause it to be read back into shared buffers. SELECT sum(reads) AS io_sum_shared_before_reads @@ -762,10 +776,15 @@ SELECT :io_sum_bulkwrite_strategy_extends_after > :io_sum_bulkwrite_strategy_ext SELECT pg_stat_have_stats('io', 0, 0); SELECT sum(evictions) + sum(reuses) + sum(extends) + sum(fsyncs) + sum(reads) + sum(writes) + sum(writebacks) + sum(hits) AS io_stats_pre_reset FROM pg_stat_io \gset +SELECT sum(evictions) + sum(reuses) + sum(extends) + sum(fsyncs) + sum(reads) + sum(writes) + sum(writebacks) + sum(hits) AS my_io_stats_pre_reset + FROM pg_my_stat_io \gset SELECT pg_stat_reset_shared('io'); SELECT sum(evictions) + sum(reuses) + sum(extends) + sum(fsyncs) + sum(reads) + sum(writes) + sum(writebacks) + sum(hits) AS io_stats_post_reset FROM pg_stat_io \gset SELECT :io_stats_post_reset < :io_stats_pre_reset; +SELECT sum(evictions) + sum(reuses) + sum(extends) + sum(fsyncs) + sum(reads) + sum(writes) + sum(writebacks) + sum(hits) AS my_io_stats_post_reset + FROM pg_my_stat_io \gset +SELECT :my_io_stats_post_reset < :my_io_stats_pre_reset; -- test BRIN index doesn't block HOT update -- 2.34.1 [text/x-diff] v4-0002-Add-pg_stat_get_backend_io.patch (14.4K, ../../[email protected]/3-v4-0002-Add-pg_stat_get_backend_io.patch) download | inline diff: From 1988e754cec0771c8fc207c266ddea9dda5e70aa Mon Sep 17 00:00:00 2001 From: Bertrand Drouvot <[email protected]> Date: Wed, 28 Aug 2024 12:59:02 +0000 Subject: [PATCH v4 2/2] Add pg_stat_get_backend_io() Adding the pg_stat_get_backend_io() function to retrieve I/O statistics for a particular backend pid. Note this function behaves as if stats_fetch_consistency is set to none. --- doc/src/sgml/monitoring.sgml | 18 ++++ src/backend/utils/activity/pgstat_io.c | 6 ++ src/backend/utils/adt/pgstatfuncs.c | 120 +++++++++++++++++++++++++ src/include/catalog/pg_proc.dat | 9 ++ src/include/pgstat.h | 1 + src/test/regress/expected/stats.out | 25 ++++++ src/test/regress/sql/stats.sql | 16 +++- 7 files changed, 194 insertions(+), 1 deletion(-) 10.9% doc/src/sgml/ 47.1% src/backend/utils/adt/ 9.2% src/include/catalog/ 15.4% src/test/regress/expected/ 14.4% src/test/regress/sql/ diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml index b28ca4e0ca..fb908172f8 100644 --- a/doc/src/sgml/monitoring.sgml +++ b/doc/src/sgml/monitoring.sgml @@ -4770,6 +4770,24 @@ description | Waiting for a newly initialized WAL file to reach durable storage </para></entry> </row> + <row> + <entry role="func_table_entry"><para role="func_signature"> + <indexterm> + <primary>pg_stat_get_backend_io</primary> + </indexterm> + <function>pg_stat_get_backend_io</function> ( <type>integer</type> ) + <returnvalue>setof record</returnvalue> + </para> + <para> + Returns I/O statistics about the backend with the specified + process ID. The output fields are exactly the same as the ones in the + <link linkend="monitoring-pg-stat-io-view"> <structname>pg_stat_io</structname></link> + view. This function behaves as if <varname>stats_fetch_consistency</varname> + is set to <literal>none</literal> (means each execution re-fetches + counters from shared memory). + </para></entry> + </row> + <row> <entry role="func_table_entry"><para role="func_signature"> <indexterm> diff --git a/src/backend/utils/activity/pgstat_io.c b/src/backend/utils/activity/pgstat_io.c index d32121fc1f..9764fd399b 100644 --- a/src/backend/utils/activity/pgstat_io.c +++ b/src/backend/utils/activity/pgstat_io.c @@ -170,6 +170,12 @@ pgstat_fetch_my_stat_io(void) return &pgStatLocal.snapshot.my_io; } +PgStat_Backend_IO * +pgstat_fetch_proc_stat_io(ProcNumber procNumber) +{ + return &pgStatLocal.shmem->io.stat[procNumber]; +} + /* * Check if there any IO stats waiting for flush. */ diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c index 4c00570396..795ff4e2f6 100644 --- a/src/backend/utils/adt/pgstatfuncs.c +++ b/src/backend/utils/adt/pgstatfuncs.c @@ -1570,6 +1570,126 @@ pg_stat_get_my_io(PG_FUNCTION_ARGS) return (Datum) 0; } +Datum +pg_stat_get_backend_io(PG_FUNCTION_ARGS) +{ + ReturnSetInfo *rsinfo; + PgStat_Backend_IO *backend_io_stats; + Datum reset_time; + ProcNumber procNumber; + PGPROC *proc; + BackendType bktype; + Datum bktype_desc; + PgStat_BktypeIO *bktype_stats; + + int backend_pid = PG_GETARG_INT32(0); + + rsinfo = (ReturnSetInfo *) fcinfo->resultinfo; + InitMaterializedSRF(fcinfo, 0); + + proc = BackendPidGetProc(backend_pid); + + /* Maybe an auxiliary process? */ + if (proc == NULL) + proc = AuxiliaryPidGetProc(backend_pid); + + /* This backend_pid does not exist */ + if (proc != NULL) + { + procNumber = GetNumberFromPGProc(proc); + backend_io_stats = pgstat_fetch_proc_stat_io(procNumber); + bktype = backend_io_stats->bktype; + reset_time = TimestampTzGetDatum(backend_io_stats->stat_reset_timestamp); + + bktype_desc = CStringGetTextDatum(GetBackendTypeDesc(bktype)); + bktype_stats = &backend_io_stats->stats; + + /* + * In Assert builds, we can afford an extra loop through all of the + * counters checking that only expected stats are non-zero, since it + * keeps the non-Assert code cleaner. + */ + Assert(pgstat_bktype_io_stats_valid(bktype_stats, bktype)); + + for (int io_obj = 0; io_obj < IOOBJECT_NUM_TYPES; io_obj++) + { + const char *obj_name = pgstat_get_io_object_name(io_obj); + + for (int io_context = 0; io_context < IOCONTEXT_NUM_TYPES; io_context++) + { + const char *context_name = pgstat_get_io_context_name(io_context); + + Datum values[IO_NUM_COLUMNS] = {0}; + bool nulls[IO_NUM_COLUMNS] = {0}; + + /* + * Some combinations of BackendType, IOObject, and IOContext + * are not valid for any type of IOOp. In such cases, omit the + * entire row from the view. + */ + if (!pgstat_tracks_io_object(bktype, io_obj, io_context)) + continue; + + values[IO_COL_BACKEND_TYPE] = bktype_desc; + values[IO_COL_CONTEXT] = CStringGetTextDatum(context_name); + values[IO_COL_OBJECT] = CStringGetTextDatum(obj_name); + if (backend_io_stats->stat_reset_timestamp != 0) + values[IO_COL_RESET_TIME] = reset_time; + else + nulls[IO_COL_RESET_TIME] = true; + + /* + * Hard-code this to the value of BLCKSZ for now. Future + * values could include XLOG_BLCKSZ, once WAL IO is tracked, + * and constant multipliers, once non-block-oriented IO (e.g. + * temporary file IO) is tracked. + */ + values[IO_COL_CONVERSION] = Int64GetDatum(BLCKSZ); + + for (int io_op = 0; io_op < IOOP_NUM_TYPES; io_op++) + { + int op_idx = pgstat_get_io_op_index(io_op); + int time_idx = pgstat_get_io_time_index(io_op); + + /* + * Some combinations of BackendType and IOOp, of IOContext + * and IOOp, and of IOObject and IOOp are not tracked. Set + * these cells in the view NULL. + */ + if (pgstat_tracks_io_op(bktype, io_obj, io_context, io_op)) + { + PgStat_Counter count = + bktype_stats->counts[io_obj][io_context][io_op]; + + values[op_idx] = Int64GetDatum(count); + } + else + nulls[op_idx] = true; + + /* not every operation is timed */ + if (time_idx == IO_COL_INVALID) + continue; + + if (!nulls[op_idx]) + { + PgStat_Counter time = + bktype_stats->times[io_obj][io_context][io_op]; + + values[time_idx] = Float8GetDatum(pg_stat_us_to_ms(time)); + } + else + nulls[time_idx] = true; + } + + tuplestore_putvalues(rsinfo->setResult, rsinfo->setDesc, + values, nulls); + } + } + } + + return (Datum) 0; +} + /* * Returns statistics of WAL activity */ diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat index 4d5af3d7cf..c4295f7c4d 100644 --- a/src/include/catalog/pg_proc.dat +++ b/src/include/catalog/pg_proc.dat @@ -5889,6 +5889,15 @@ proargnames => '{backend_type,object,context,reads,read_time,writes,write_time,writebacks,writeback_time,extends,extend_time,op_bytes,hits,evictions,reuses,fsyncs,fsync_time,stats_reset}', prosrc => 'pg_stat_get_my_io' }, +{ oid => '8896', descr => 'statistics: per backend type IO statistics', + proname => 'pg_stat_get_backend_io', prorows => '30', proretset => 't', + provolatile => 'v', proparallel => 'r', prorettype => 'record', + proargtypes => 'int4', + proallargtypes => '{int4,text,text,text,int8,float8,int8,float8,int8,float8,int8,float8,int8,int8,int8,int8,int8,float8,timestamptz}', + proargmodes => '{i,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o}', + proargnames => '{backend_pid,backend_type,object,context,reads,read_time,writes,write_time,writebacks,writeback_time,extends,extend_time,op_bytes,hits,evictions,reuses,fsyncs,fsync_time,stats_reset}', + prosrc => 'pg_stat_get_backend_io' }, + { oid => '1136', descr => 'statistics: information about WAL activity', proname => 'pg_stat_get_wal', proisstrict => 'f', provolatile => 's', proparallel => 'r', prorettype => 'record', proargtypes => '', diff --git a/src/include/pgstat.h b/src/include/pgstat.h index 9d63b1a5b7..2a2b4a2947 100644 --- a/src/include/pgstat.h +++ b/src/include/pgstat.h @@ -570,6 +570,7 @@ extern void pgstat_count_io_op_time(IOObject io_object, IOContext io_context, extern PgStat_IO *pgstat_fetch_global_stat_io(void); extern PgStat_Backend_IO *pgstat_fetch_my_stat_io(void); +extern PgStat_Backend_IO *pgstat_fetch_proc_stat_io(ProcNumber procNumber); extern const char *pgstat_get_io_context_name(IOContext io_context); extern const char *pgstat_get_io_object_name(IOObject io_object); diff --git a/src/test/regress/expected/stats.out b/src/test/regress/expected/stats.out index c489e528e0..aced015c2f 100644 --- a/src/test/regress/expected/stats.out +++ b/src/test/regress/expected/stats.out @@ -1263,12 +1263,18 @@ SELECT sum(extends) AS io_sum_shared_before_extends FROM pg_stat_io WHERE context = 'normal' AND object = 'relation' \gset SELECT sum(extends) AS my_io_sum_shared_before_extends FROM pg_my_stat_io WHERE context = 'normal' AND object = 'relation' \gset +SELECT sum(extends) AS backend_io_sum_shared_before_extends + FROM pg_stat_get_backend_io(pg_backend_pid()) + WHERE context = 'normal' AND object = 'relation' \gset SELECT sum(writes) AS writes, sum(fsyncs) AS fsyncs FROM pg_stat_io WHERE object = 'relation' \gset io_sum_shared_before_ SELECT sum(writes) AS writes, sum(fsyncs) AS fsyncs FROM pg_my_stat_io WHERE object = 'relation' \gset my_io_sum_shared_before_ +SELECT sum(writes) AS writes, sum(fsyncs) AS fsyncs + FROM pg_stat_get_backend_io(pg_backend_pid()) + WHERE object = 'relation' \gset backend_io_sum_shared_before_ CREATE TABLE test_io_shared(a int); INSERT INTO test_io_shared SELECT i FROM generate_series(1,100)i; SELECT pg_stat_force_next_flush(); @@ -1293,6 +1299,15 @@ SELECT :my_io_sum_shared_after_extends > :my_io_sum_shared_before_extends; t (1 row) +SELECT sum(extends) AS backend_io_sum_shared_after_extends + FROM pg_stat_get_backend_io(pg_backend_pid()) + WHERE context = 'normal' AND object = 'relation' \gset +SELECT :backend_io_sum_shared_after_extends > :backend_io_sum_shared_before_extends; + ?column? +---------- + t +(1 row) + -- After a checkpoint, there should be some additional IOCONTEXT_NORMAL writes -- and fsyncs in the global stats (not for the backend). -- See comment above for rationale for two explicit CHECKPOINTs. @@ -1553,6 +1568,8 @@ SELECT sum(evictions) + sum(reuses) + sum(extends) + sum(fsyncs) + sum(reads) + FROM pg_stat_io \gset SELECT sum(evictions) + sum(reuses) + sum(extends) + sum(fsyncs) + sum(reads) + sum(writes) + sum(writebacks) + sum(hits) AS my_io_stats_pre_reset FROM pg_my_stat_io \gset +SELECT sum(evictions) + sum(reuses) + sum(extends) + sum(fsyncs) + sum(reads) + sum(writes) + sum(writebacks) + sum(hits) AS backend_io_stats_pre_reset + FROM pg_stat_get_backend_io(pg_backend_pid()) \gset SELECT pg_stat_reset_shared('io'); pg_stat_reset_shared ---------------------- @@ -1575,6 +1592,14 @@ SELECT :my_io_stats_post_reset < :my_io_stats_pre_reset; t (1 row) +SELECT sum(evictions) + sum(reuses) + sum(extends) + sum(fsyncs) + sum(reads) + sum(writes) + sum(writebacks) + sum(hits) AS backend_io_stats_post_reset + FROM pg_stat_get_backend_io(pg_backend_pid()) \gset +SELECT :backend_io_stats_post_reset < :backend_io_stats_pre_reset; + ?column? +---------- + t +(1 row) + -- test BRIN index doesn't block HOT update CREATE TABLE brin_hot ( id integer PRIMARY KEY, diff --git a/src/test/regress/sql/stats.sql b/src/test/regress/sql/stats.sql index c95cb71652..d05009e1f5 100644 --- a/src/test/regress/sql/stats.sql +++ b/src/test/regress/sql/stats.sql @@ -611,12 +611,18 @@ SELECT sum(extends) AS io_sum_shared_before_extends FROM pg_stat_io WHERE context = 'normal' AND object = 'relation' \gset SELECT sum(extends) AS my_io_sum_shared_before_extends FROM pg_my_stat_io WHERE context = 'normal' AND object = 'relation' \gset +SELECT sum(extends) AS backend_io_sum_shared_before_extends + FROM pg_stat_get_backend_io(pg_backend_pid()) + WHERE context = 'normal' AND object = 'relation' \gset SELECT sum(writes) AS writes, sum(fsyncs) AS fsyncs FROM pg_stat_io WHERE object = 'relation' \gset io_sum_shared_before_ SELECT sum(writes) AS writes, sum(fsyncs) AS fsyncs FROM pg_my_stat_io WHERE object = 'relation' \gset my_io_sum_shared_before_ +SELECT sum(writes) AS writes, sum(fsyncs) AS fsyncs + FROM pg_stat_get_backend_io(pg_backend_pid()) + WHERE object = 'relation' \gset backend_io_sum_shared_before_ CREATE TABLE test_io_shared(a int); INSERT INTO test_io_shared SELECT i FROM generate_series(1,100)i; SELECT pg_stat_force_next_flush(); @@ -626,6 +632,10 @@ SELECT :io_sum_shared_after_extends > :io_sum_shared_before_extends; SELECT sum(extends) AS my_io_sum_shared_after_extends FROM pg_my_stat_io WHERE context = 'normal' AND object = 'relation' \gset SELECT :my_io_sum_shared_after_extends > :my_io_sum_shared_before_extends; +SELECT sum(extends) AS backend_io_sum_shared_after_extends + FROM pg_stat_get_backend_io(pg_backend_pid()) + WHERE context = 'normal' AND object = 'relation' \gset +SELECT :backend_io_sum_shared_after_extends > :backend_io_sum_shared_before_extends; -- After a checkpoint, there should be some additional IOCONTEXT_NORMAL writes -- and fsyncs in the global stats (not for the backend). @@ -778,6 +788,8 @@ SELECT sum(evictions) + sum(reuses) + sum(extends) + sum(fsyncs) + sum(reads) + FROM pg_stat_io \gset SELECT sum(evictions) + sum(reuses) + sum(extends) + sum(fsyncs) + sum(reads) + sum(writes) + sum(writebacks) + sum(hits) AS my_io_stats_pre_reset FROM pg_my_stat_io \gset +SELECT sum(evictions) + sum(reuses) + sum(extends) + sum(fsyncs) + sum(reads) + sum(writes) + sum(writebacks) + sum(hits) AS backend_io_stats_pre_reset + FROM pg_stat_get_backend_io(pg_backend_pid()) \gset SELECT pg_stat_reset_shared('io'); SELECT sum(evictions) + sum(reuses) + sum(extends) + sum(fsyncs) + sum(reads) + sum(writes) + sum(writebacks) + sum(hits) AS io_stats_post_reset FROM pg_stat_io \gset @@ -785,7 +797,9 @@ SELECT :io_stats_post_reset < :io_stats_pre_reset; SELECT sum(evictions) + sum(reuses) + sum(extends) + sum(fsyncs) + sum(reads) + sum(writes) + sum(writebacks) + sum(hits) AS my_io_stats_post_reset FROM pg_my_stat_io \gset SELECT :my_io_stats_post_reset < :my_io_stats_pre_reset; - +SELECT sum(evictions) + sum(reuses) + sum(extends) + sum(fsyncs) + sum(reads) + sum(writes) + sum(writebacks) + sum(hits) AS backend_io_stats_post_reset + FROM pg_stat_get_backend_io(pg_backend_pid()) \gset +SELECT :backend_io_stats_post_reset < :backend_io_stats_pre_reset; -- test BRIN index doesn't block HOT update CREATE TABLE brin_hot ( -- 2.34.1 ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: per backend I/O statistics @ 2024-09-17 11:52 Nazir Bilal Yavuz <[email protected]> parent: Bertrand Drouvot <[email protected]> 0 siblings, 1 reply; 32+ messages in thread From: Nazir Bilal Yavuz @ 2024-09-17 11:52 UTC (permalink / raw) To: Bertrand Drouvot <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected] Hi, On Fri, 13 Sept 2024 at 19:09, Bertrand Drouvot <[email protected]> wrote: > On Fri, Sep 13, 2024 at 04:45:08PM +0300, Nazir Bilal Yavuz wrote: > > - The pgstat_reset_io_counter_internal() is called in the > > pgstat_shutdown_hook(). This causes the stats_reset column showing the > > termination time of the old backend when its proc num is reassigned to > > a new backend. > > doh! Nice catch, thanks! > > And also new backends that are not re-using a previous "existing" process slot > are getting the last reset time (if any). So the right place to fix this is in > pgstat_io_init_backend_cb(): done in v4 attached. v4 also sets the reset time to > 0 in pgstat_shutdown_hook() (but that's not necessary though, that's just to be > right "conceptually" speaking). Thanks, I confirm that it is fixed. You mentioned some refactoring upthread: On Fri, 6 Sept 2024 at 18:03, Bertrand Drouvot <[email protected]> wrote: > > - If we agree on the current proposal then I'll do some refactoring in > pg_stat_get_backend_io(), pg_stat_get_my_io() and pg_stat_get_io() to avoid > duplicated code (it's not done yet to ease the review). Could we remove pg_stat_get_my_io() completely and use pg_stat_get_backend_io() with the current backend's pid to get the current backend's stats? If you meant the same thing, please don't mind it. -- Regards, Nazir Bilal Yavuz Microsoft ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: per backend I/O statistics @ 2024-09-17 13:07 Bertrand Drouvot <[email protected]> parent: Nazir Bilal Yavuz <[email protected]> 0 siblings, 1 reply; 32+ messages in thread From: Bertrand Drouvot @ 2024-09-17 13:07 UTC (permalink / raw) To: Nazir Bilal Yavuz <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected] Hi, On Tue, Sep 17, 2024 at 02:52:01PM +0300, Nazir Bilal Yavuz wrote: > Hi, > > On Fri, 13 Sept 2024 at 19:09, Bertrand Drouvot > <[email protected]> wrote: > > On Fri, Sep 13, 2024 at 04:45:08PM +0300, Nazir Bilal Yavuz wrote: > > > - The pgstat_reset_io_counter_internal() is called in the > > > pgstat_shutdown_hook(). This causes the stats_reset column showing the > > > termination time of the old backend when its proc num is reassigned to > > > a new backend. > > > > doh! Nice catch, thanks! > > > > And also new backends that are not re-using a previous "existing" process slot > > are getting the last reset time (if any). So the right place to fix this is in > > pgstat_io_init_backend_cb(): done in v4 attached. v4 also sets the reset time to > > 0 in pgstat_shutdown_hook() (but that's not necessary though, that's just to be > > right "conceptually" speaking). > > Thanks, I confirm that it is fixed. Thanks! > You mentioned some refactoring upthread: > > On Fri, 6 Sept 2024 at 18:03, Bertrand Drouvot > <[email protected]> wrote: > > > > - If we agree on the current proposal then I'll do some refactoring in > > pg_stat_get_backend_io(), pg_stat_get_my_io() and pg_stat_get_io() to avoid > > duplicated code (it's not done yet to ease the review). > > Could we remove pg_stat_get_my_io() completely and use > pg_stat_get_backend_io() with the current backend's pid to get the > current backend's stats? The reason why I keep pg_stat_get_my_io() is because (as mentioned in [1]), the statistics snapshot is build for "my backend stats" (means it depends of the stats_fetch_consistency value). I can see use cases for that. OTOH, pg_stat_get_backend_io() behaves as if stats_fetch_consistency is set to none (each execution re-fetches counters from shared memory) (see [2]). Indeed, the snapshot is not build in each backend to copy all the others backends stats, as 1/ I think that there is no use case (there is no need to get others backends I/O statistics while taking care of the stats_fetch_consistency) and 2/ that could be memory expensive depending of the number of max connections. So I think it's better to keep both functions as they behave differently. Thoughts? > If you meant the same thing, please don't > mind it. What I meant to say is to try to remove the duplicate code that we can find in those 3 functions (say creating a new function that would contain the duplicate code and make use of this new function in the 3 others). Did not look at it in details yet. [1]: https://www.postgresql.org/message-id/ZtXR%2BCtkEVVE/LHF%40ip-10-97-1-34.eu-west-3.compute.internal [2]: https://www.postgresql.org/message-id/ZtsZtaRza9bFFeF8%40ip-10-97-1-34.eu-west-3.compute.internal Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: per backend I/O statistics @ 2024-09-17 13:47 Nazir Bilal Yavuz <[email protected]> parent: Bertrand Drouvot <[email protected]> 0 siblings, 1 reply; 32+ messages in thread From: Nazir Bilal Yavuz @ 2024-09-17 13:47 UTC (permalink / raw) To: Bertrand Drouvot <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected] Hi, On Tue, 17 Sept 2024 at 16:07, Bertrand Drouvot <[email protected]> wrote: > On Tue, Sep 17, 2024 at 02:52:01PM +0300, Nazir Bilal Yavuz wrote: > > Could we remove pg_stat_get_my_io() completely and use > > pg_stat_get_backend_io() with the current backend's pid to get the > > current backend's stats? > > The reason why I keep pg_stat_get_my_io() is because (as mentioned in [1]), the > statistics snapshot is build for "my backend stats" (means it depends of the > stats_fetch_consistency value). I can see use cases for that. > > OTOH, pg_stat_get_backend_io() behaves as if stats_fetch_consistency is set to > none (each execution re-fetches counters from shared memory) (see [2]). Indeed, > the snapshot is not build in each backend to copy all the others backends stats, > as 1/ I think that there is no use case (there is no need to get others backends > I/O statistics while taking care of the stats_fetch_consistency) and 2/ that > could be memory expensive depending of the number of max connections. > > So I think it's better to keep both functions as they behave differently. > > Thoughts? Yes, that is correct. Sorry, you already had explained it and I had agreed with it but I forgot. > > If you meant the same thing, please don't > > mind it. > > What I meant to say is to try to remove the duplicate code that we can find in > those 3 functions (say creating a new function that would contain the duplicate > code and make use of this new function in the 3 others). Did not look at it in > details yet. I got it, thanks for the explanation. -- Regards, Nazir Bilal Yavuz Microsoft ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: per backend I/O statistics @ 2024-09-17 13:56 Bertrand Drouvot <[email protected]> parent: Nazir Bilal Yavuz <[email protected]> 0 siblings, 1 reply; 32+ messages in thread From: Bertrand Drouvot @ 2024-09-17 13:56 UTC (permalink / raw) To: Nazir Bilal Yavuz <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected] Hi, On Tue, Sep 17, 2024 at 04:47:51PM +0300, Nazir Bilal Yavuz wrote: > Hi, > > On Tue, 17 Sept 2024 at 16:07, Bertrand Drouvot > <[email protected]> wrote: > > So I think it's better to keep both functions as they behave differently. > > > > Thoughts? > > Yes, that is correct. Sorry, you already had explained it and I had > agreed with it but I forgot. No problem at all! (I re-explained because I'm not always 100% sure that my explanations are crystal clear ;-) ) Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: per backend I/O statistics @ 2024-09-20 04:26 Michael Paquier <[email protected]> parent: Bertrand Drouvot <[email protected]> 0 siblings, 1 reply; 32+ messages in thread From: Michael Paquier @ 2024-09-20 04:26 UTC (permalink / raw) To: Bertrand Drouvot <[email protected]>; +Cc: Nazir Bilal Yavuz <[email protected]>; Alvaro Herrera <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected] On Tue, Sep 17, 2024 at 01:56:34PM +0000, Bertrand Drouvot wrote: > No problem at all! (I re-explained because I'm not always 100% sure that my > explanations are crystal clear ;-) ) We've discussed a bit this patch offline, but after studying the patch I doubt that this part is a good idea: + /* has to be at the end due to FLEXIBLE_ARRAY_MEMBER */ + PgStatShared_IO io; } PgStat_ShmemControl; We are going to be in trouble if we introduce a second member in this routine that has a FLEXIBLE_ARRAY_MEMBER, because PgStat_ShmemControl relies on the fact that all its members after deterministic offset positions in this structure. So this lacks flexibility. This choice is caused by the fact that we don't exactly know the number of backends because that's controlled by the PGC_POSTMASTER GUC max_connections so the size of the structure would be undefined. There is a parallel with replication slot statistics here, where we save the replication slot data in the dshash based on their index number in shmem. Hence, wouldn't it be better to do like replication slot stats, where we use the dshash and a key based on the procnum of each backend or auxiliary process (ProcNumber in procnumber.h)? If at restart max_connections is lower than what was previously used, we could discard entries that would not fit anymore into the charts. This is probably not something that happens often, so I'm not really worried about forcing the removal of these stats depending on how the upper-bound of ProcNumber evolves. So, using a new kind of ID and making this kind variable-numbered may ease the implementation quite a bit, while avoiding any extensibility issues with the shmem portion of the patch if these are fixed-numbered. The reporting of these stats comes down to having a parallel with pgstat_count_io_op_time(), but to make sure that the stats are split by connection slot number rather than the current split of pg_stat_io. All its callers are in localbuf.c, bufmgr.c and md.c, living with some duplication in the code paths to gather the stats may be OK. pg_stat_get_my_io() is based on a O(n^3). IOOBJECT_NUM_TYPES is fortunately low, still that's annoying. This would rely on the fact that we would use the ProcNumber for the dshash key, and this information is not provided in pg_stat_activity. Perhaps we should add this information in pg_stat_activity so as it would be easily possible to do joins with a SQL function that returns a SRF with all the stats associated with a given connection slot (auxiliary or backend process)? That would be a separate patch. Perhaps that's even something that has popped up for the work with threading (did not follow this part closely, TBH)? The active PIDs of the live sessions are not stored in the active stats, why not? Perhaps that's useless anyway if we expose the ProcNumbers in pg_stat_activity and make the stats available with a single function taking in input a ProcNumber. Just mentioning an option to consider. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: per backend I/O statistics @ 2024-10-07 09:54 Bertrand Drouvot <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 1 reply; 32+ messages in thread From: Bertrand Drouvot @ 2024-10-07 09:54 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: Nazir Bilal Yavuz <[email protected]>; Alvaro Herrera <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected] Hi, On Fri, Sep 20, 2024 at 01:26:49PM +0900, Michael Paquier wrote: > On Tue, Sep 17, 2024 at 01:56:34PM +0000, Bertrand Drouvot wrote: > > No problem at all! (I re-explained because I'm not always 100% sure that my > > explanations are crystal clear ;-) ) > > We've discussed a bit this patch offline, but after studying the patch > I doubt that this part is a good idea: > > + /* has to be at the end due to FLEXIBLE_ARRAY_MEMBER */ > + PgStatShared_IO io; > } PgStat_ShmemControl; > > We are going to be in trouble if we introduce a second member in this > routine that has a FLEXIBLE_ARRAY_MEMBER, because PgStat_ShmemControl > relies on the fact that all its members after deterministic offset > positions in this structure. Agree that it would be an issue should we have to add a new FLEXIBLE_ARRAY_MEMBER. > So this lacks flexibility. This choice > is caused by the fact that we don't exactly know the number of > backends because that's controlled by the PGC_POSTMASTER GUC > max_connections so the size of the structure would be undefined. Right. > There is a parallel with replication slot statistics here, where we > save the replication slot data in the dshash based on their index > number in shmem. Hence, wouldn't it be better to do like replication > slot stats, where we use the dshash and a key based on the procnum of > each backend or auxiliary process (ProcNumber in procnumber.h)? If at > restart max_connections is lower than what was previously used, we > could discard entries that would not fit anymore into the charts. > This is probably not something that happens often, so I'm not really > worried about forcing the removal of these stats depending on how the > upper-bound of ProcNumber evolves. Yeah, I'll look at implementing the dshash based on their procnum and see where it goes. > So, using a new kind of ID and making this kind variable-numbered may > ease the implementation quite a bit, while avoiding any extensibility > issues with the shmem portion of the patch if these are > fixed-numbered. Agree. > This would rely on the fact that we would use the ProcNumber for the > dshash key, and this information is not provided in pg_stat_activity. > Perhaps we should add this information in pg_stat_activity so as it > would be easily possible to do joins with a SQL function that returns > a SRF with all the stats associated with a given connection slot > (auxiliary or backend process)? I'm not sure that's needed. What has been done in the previous versions is to get the stats based on the pid (see pg_stat_get_backend_io()) where the procnumber is retrieved with something like GetNumberFromPGProc(BackendPidGetProc(pid)). Do you see an issue with that approach? > The active PIDs of the live sessions are not stored in the active > stats, why not? That was not needed. We can still retrieve the stats based on the pid thanks to something like GetNumberFromPGProc(BackendPidGetProc(pid)) without having to actually store the pid in the stats. I think that's fine because the pid only matters at "display" time (pg_stat_get_backend_io()). Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: per backend I/O statistics @ 2024-10-08 04:46 Michael Paquier <[email protected]> parent: Bertrand Drouvot <[email protected]> 0 siblings, 1 reply; 32+ messages in thread From: Michael Paquier @ 2024-10-08 04:46 UTC (permalink / raw) To: Bertrand Drouvot <[email protected]>; +Cc: Nazir Bilal Yavuz <[email protected]>; Alvaro Herrera <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected] On Mon, Oct 07, 2024 at 09:54:21AM +0000, Bertrand Drouvot wrote: > On Fri, Sep 20, 2024 at 01:26:49PM +0900, Michael Paquier wrote: >> This would rely on the fact that we would use the ProcNumber for the >> dshash key, and this information is not provided in pg_stat_activity. >> Perhaps we should add this information in pg_stat_activity so as it >> would be easily possible to do joins with a SQL function that returns >> a SRF with all the stats associated with a given connection slot >> (auxiliary or backend process)? > > I'm not sure that's needed. What has been done in the previous versions is > to get the stats based on the pid (see pg_stat_get_backend_io()) where the > procnumber is retrieved with something like GetNumberFromPGProc(BackendPidGetProc(pid)). Ah, I see. So you could just have the proc number in the key to control the upper-bound on the number of possible stats entries in the dshash. Assuming that none of this data is persisted to the stats file at shutdown and that the stats of a single entry are reset each time a new backend reuses a previous proc slot, that would be OK by me, I guess. >> The active PIDs of the live sessions are not stored in the active >> stats, why not? > > That was not needed. We can still retrieve the stats based on the pid thanks > to something like GetNumberFromPGProc(BackendPidGetProc(pid)) without having > to actually store the pid in the stats. I think that's fine because the pid > only matters at "display" time (pg_stat_get_backend_io()). Okay, per the above and the persistency of the stats. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: per backend I/O statistics @ 2024-10-08 16:28 Bertrand Drouvot <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 1 reply; 32+ messages in thread From: Bertrand Drouvot @ 2024-10-08 16:28 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: Nazir Bilal Yavuz <[email protected]>; Alvaro Herrera <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected] Hi, On Tue, Oct 08, 2024 at 01:46:23PM +0900, Michael Paquier wrote: > On Mon, Oct 07, 2024 at 09:54:21AM +0000, Bertrand Drouvot wrote: > > On Fri, Sep 20, 2024 at 01:26:49PM +0900, Michael Paquier wrote: > >> This would rely on the fact that we would use the ProcNumber for the > >> dshash key, and this information is not provided in pg_stat_activity. > >> Perhaps we should add this information in pg_stat_activity so as it > >> would be easily possible to do joins with a SQL function that returns > >> a SRF with all the stats associated with a given connection slot > >> (auxiliary or backend process)? > > > > I'm not sure that's needed. What has been done in the previous versions is > > to get the stats based on the pid (see pg_stat_get_backend_io()) where the > > procnumber is retrieved with something like GetNumberFromPGProc(BackendPidGetProc(pid)). > > Ah, I see. So you could just have the proc number in the key to > control the upper-bound on the number of possible stats entries in the > dshash. Yes. > Assuming that none of this data is persisted to the stats file at > shutdown and that the stats of a single entry are reset each time a > new backend reuses a previous proc slot, that would be OK by me, I > guess. > > >> The active PIDs of the live sessions are not stored in the active > >> stats, why not? > > > > That was not needed. We can still retrieve the stats based on the pid thanks > > to something like GetNumberFromPGProc(BackendPidGetProc(pid)) without having > > to actually store the pid in the stats. I think that's fine because the pid > > only matters at "display" time (pg_stat_get_backend_io()). > > Okay, per the above and the persistency of the stats. Great, I'll work on an updated patch version then. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: per backend I/O statistics @ 2024-10-31 05:09 Bertrand Drouvot <[email protected]> parent: Bertrand Drouvot <[email protected]> 0 siblings, 1 reply; 32+ messages in thread From: Bertrand Drouvot @ 2024-10-31 05:09 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: Nazir Bilal Yavuz <[email protected]>; Alvaro Herrera <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected] Hi, On Tue, Oct 08, 2024 at 04:28:39PM +0000, Bertrand Drouvot wrote: > > > On Fri, Sep 20, 2024 at 01:26:49PM +0900, Michael Paquier wrote: > > > > Okay, per the above and the persistency of the stats. > > Great, I'll work on an updated patch version then. > I spend some time on this during the last 2 days and I think we have 3 design options. === GOALS === But first let's sump up the goals that I think we agreed on: - Keep pg_stat_io as it is today: give the whole server picture and serialize the stats to disk. - Introduce per-backend IO stats and 2 new APIs to: 1. Provide the IO stats for "my backend" (through say pg_my_stat_io), this would take care of the stats_fetch_consistency. 2. Retrieve the IO stats for another backend (through say pg_stat_get_backend_io(pid)) that would _not_ take care of stats_fetch_consistency, as: 2.1/ I think that there is no use case (there is no need to get others backends I/O statistics while taking care of the stats_fetch_consistency) 2.2/ That could be memory expensive to store a snapshot for all the backends (depending of the number of backend created) - There is no need to serialize the per-backend IO stats to disk (no point to see stats for backends that do not exist anymore after a re-start). - The per-backend IO stats should be variable-numbered (not fixed), as per up-thread discussion. === OPTIONS === So, based on this, I think that we could: Option 1: "move" the existing PGSTAT_KIND_IO to variable-numbered and let this KIND take care of the aggregated view (pg_stat_io) and the per-backend stats. Option 2: let PGSTAT_KIND_IO as it is and introduce a new PGSTAT_KIND_BACKEND_IO that would be variable-numbered. Option 3: Remove PGSTAT_KIND_IO, introduce a new PGSTAT_KIND_BACKEND_IO that would be variable-numbered and store the "aggregated stats aka pg_stat_io" in shared memory (not part of the variable-numbered hash). Per-backend stats could be aggregated into "pg_stat_io" during the flush_pending_cb call for example. === BEST OPTION? === I would opt for Option 2 as: - The stats system is currently not designed for Option 1 and our goals (for example the shared_data_len is used to serialize but also to fetch the entries, see pgstat_fetch_entry()) so that would need some hack to serialize only a part of them and still be able to fetch them all). - Mixing "fixed" and "variable" in the same KIND does not sound like a good idea (though that might be possible with some hacks, I don't think that would be easy to maintain). - Having the per-backend as "variable" in its dedicated kind looks more reasonable and less error-prone. - I don't think there is a stats design similar to option 3 currently, so I'm not sure there is a need to develop something new while Option 2 could be done. - Option 3 would need some hack for (at least) the "pg_stat_io" [de]serialization part. - Option 2 seems to offer more flexibility (as compare to Option 1 and 3). Thoughts? Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: per backend I/O statistics @ 2024-11-04 10:01 Bertrand Drouvot <[email protected]> parent: Bertrand Drouvot <[email protected]> 0 siblings, 1 reply; 32+ messages in thread From: Bertrand Drouvot @ 2024-11-04 10:01 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: Nazir Bilal Yavuz <[email protected]>; Alvaro Herrera <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected] Hi, On Thu, Oct 31, 2024 at 05:09:56AM +0000, Bertrand Drouvot wrote: > === OPTIONS === > > So, based on this, I think that we could: > > Option 1: "move" the existing PGSTAT_KIND_IO to variable-numbered and let this > KIND take care of the aggregated view (pg_stat_io) and the per-backend stats. > > Option 2: let PGSTAT_KIND_IO as it is and introduce a new PGSTAT_KIND_BACKEND_IO > that would be variable-numbered. > > Option 3: Remove PGSTAT_KIND_IO, introduce a new PGSTAT_KIND_BACKEND_IO that > would be variable-numbered and store the "aggregated stats aka pg_stat_io" in > shared memory (not part of the variable-numbered hash). Per-backend stats > could be aggregated into "pg_stat_io" during the flush_pending_cb call for example. > > === BEST OPTION? === > > I would opt for Option 2 as: > > - The stats system is currently not designed for Option 1 and our goals (for > example the shared_data_len is used to serialize but also to fetch the entries, > see pgstat_fetch_entry()) so that would need some hack to serialize only a part > of them and still be able to fetch them all). > > - Mixing "fixed" and "variable" in the same KIND does not sound like a good idea > (though that might be possible with some hacks, I don't think that would be > easy to maintain). > > - Having the per-backend as "variable" in its dedicated kind looks more reasonable > and less error-prone. > > - I don't think there is a stats design similar to option 3 currently, so I'm > not sure there is a need to develop something new while Option 2 could be done. > > - Option 3 would need some hack for (at least) the "pg_stat_io" [de]serialization > part. > > - Option 2 seems to offer more flexibility (as compare to Option 1 and 3). > > Thoughts? And why not add more per-backend stats in the future? (once the I/O part is done). I think that's one more reason to go with option 2 (and implementing a brand new PGSTAT_KIND_BACKEND kind). Thoughts? Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: per backend I/O statistics @ 2024-11-05 17:37 Bertrand Drouvot <[email protected]> parent: Bertrand Drouvot <[email protected]> 0 siblings, 1 reply; 32+ messages in thread From: Bertrand Drouvot @ 2024-11-05 17:37 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: Nazir Bilal Yavuz <[email protected]>; Alvaro Herrera <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected] Hi, On Mon, Nov 04, 2024 at 10:01:50AM +0000, Bertrand Drouvot wrote: > And why not add more per-backend stats in the future? (once the I/O part is done). > > I think that's one more reason to go with option 2 (and implementing a brand new > PGSTAT_KIND_BACKEND kind). I'm starting working on option 2, I think it will be easier to discuss with a patch proposal to look at. If in the meantime, one strongly disagree with option 2 (means implement a brand new PGSTAT_KIND_BACKEND and keep PGSTAT_KIND_IO), please let me know. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: per backend I/O statistics @ 2024-11-05 23:39 Michael Paquier <[email protected]> parent: Bertrand Drouvot <[email protected]> 0 siblings, 1 reply; 32+ messages in thread From: Michael Paquier @ 2024-11-05 23:39 UTC (permalink / raw) To: Bertrand Drouvot <[email protected]>; +Cc: Nazir Bilal Yavuz <[email protected]>; Alvaro Herrera <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected] On Tue, Nov 05, 2024 at 05:37:15PM +0000, Bertrand Drouvot wrote: > I'm starting working on option 2, I think it will be easier to discuss with > a patch proposal to look at. > > If in the meantime, one strongly disagree with option 2 (means implement a brand > new PGSTAT_KIND_BACKEND and keep PGSTAT_KIND_IO), please let me know. Sorry for the late reply, catching up a bit. As you are quoting in [1], you do not expect the backend-io stats and the more global pg_stat_io to achieve the same level of consistency as the backend stats would be gone at restart, and wiped out when a backend shuts down. So, splitting them with a different stats kind feels more natural because it would be possible to control how each stat kind behaves depending on the code shutdown and reset paths within their own callbacks rather than making the callbacks of PGSTAT_KIND_IO more complex than they already are. And pg_stat_io is a fixed-numbered stats kind because of the way it aggregates its stats with a number states defined at compile-time. Is the structure you have in mind different than PgStat_BktypeIO? Perhaps a split is better anyway with that in mind. The amount of memory required to store the snapshots of backend-IO does not worry me much, TBH, but you are worried about a high turnover of connections that could cause a lot of bloat in the backend-IO snapshots because of the persistency that these stats would have, right? Actually, we already have cases with other stats kinds where it is possible for a backend to hold references to some stats on an object that has been dropped concurrently. At least, that's possible when a backend shuts down. If possible, supporting snapshots would be more consistent with the other stats. Just to be clear, I am not in favor of making PgStat_HashKey larger than it already is. With 8 bytes allocated for the object ID, the chances of conflicts in the dshash for a single variable-numbered stats kind play in our favor already even with a couple of million entries. [1]: https://www.postgresql.org/message-id/[email protected] -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: per backend I/O statistics @ 2024-11-06 13:51 Bertrand Drouvot <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 1 reply; 32+ messages in thread From: Bertrand Drouvot @ 2024-11-06 13:51 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: Nazir Bilal Yavuz <[email protected]>; Alvaro Herrera <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected] Hi, On Wed, Nov 06, 2024 at 08:39:07AM +0900, Michael Paquier wrote: > On Tue, Nov 05, 2024 at 05:37:15PM +0000, Bertrand Drouvot wrote: > > I'm starting working on option 2, I think it will be easier to discuss with > > a patch proposal to look at. > > > > If in the meantime, one strongly disagree with option 2 (means implement a brand > > new PGSTAT_KIND_BACKEND and keep PGSTAT_KIND_IO), please let me know. > > Sorry for the late reply, catching up a bit. No problem at all, thanks for looking at it! > As you are quoting in [1], you do not expect the backend-io stats and > the more global pg_stat_io to achieve the same level of consistency as > the backend stats would be gone at restart, and wiped out when a > backend shuts down. Yes. > So, splitting them with a different stats kind > feels more natural because it would be possible to control how each > stat kind behaves depending on the code shutdown and reset paths > within their own callbacks rather than making the callbacks of > PGSTAT_KIND_IO more complex than they already are. Yeah, thanks for sharing your thoughts. > And pg_stat_io is > a fixed-numbered stats kind because of the way it aggregates its stats > with a number states defined at compile-time. > > Is the structure you have in mind different than PgStat_BktypeIO? Very close. > Perhaps a split is better anyway with that in mind. The in-progress patch (not shared yet) is using the following: " typedef struct PgStat_Backend { TimestampTz stat_reset_timestamp; BackendType bktype; PgStat_BktypeIO stats; } PgStat_Backend; " The bktype is used to be able to filter the stats correctly when we display them. > The amount of memory required to store the snapshots of backend-IO > does not worry me much, TBH, but you are worried about a high turnover > of connections that could cause a lot of bloat in the backend-IO > snapshots because of the persistency that these stats would have, > right? Not only a high turnover but also a high number of entries created in the hash. Furthermore I don't see any use case of relying on stats_fetch_consistency while querying other backend's stats. > If possible, supporting snapshots would be > more consistent with the other stats. I have I mind to support the snapshots _only_ when querying our own stats. I can measure the memory impact if we use them also when querying other backends stats too (though I don't see a use case). > Just to be clear, I am not in favor of making PgStat_HashKey larger > than it already is. That's not needed, the patch I'm working on stores the proc number in the objid field of the key. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: per backend I/O statistics @ 2024-11-07 00:50 Michael Paquier <[email protected]> parent: Bertrand Drouvot <[email protected]> 0 siblings, 1 reply; 32+ messages in thread From: Michael Paquier @ 2024-11-07 00:50 UTC (permalink / raw) To: Bertrand Drouvot <[email protected]>; +Cc: Nazir Bilal Yavuz <[email protected]>; Alvaro Herrera <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected] On Wed, Nov 06, 2024 at 01:51:02PM +0000, Bertrand Drouvot wrote: > That's not needed, the patch I'm working on stores the proc number in the > objid field of the key. Relying on the procnumber for the object ID gets a +1 here. That provides an automatic cap on the maximum number of entries that can exist at once for this new stats kind. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: per backend I/O statistics @ 2024-11-07 16:32 Bertrand Drouvot <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 1 reply; 32+ messages in thread From: Bertrand Drouvot @ 2024-11-07 16:32 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: Nazir Bilal Yavuz <[email protected]>; Alvaro Herrera <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected] Hi, On Thu, Nov 07, 2024 at 09:50:59AM +0900, Michael Paquier wrote: > On Wed, Nov 06, 2024 at 01:51:02PM +0000, Bertrand Drouvot wrote: > > That's not needed, the patch I'm working on stores the proc number in the > > objid field of the key. > > Relying on the procnumber for the object ID gets a +1 here. Thanks! > That > provides an automatic cap on the maximum number of entries that can > exist at once for this new stats kind. Yeah. POC patch is now working, will wear a reviewer hat now and will share in one or 2 days. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: per backend I/O statistics @ 2024-11-08 14:09 Bertrand Drouvot <[email protected]> parent: Bertrand Drouvot <[email protected]> 0 siblings, 1 reply; 32+ messages in thread From: Bertrand Drouvot @ 2024-11-08 14:09 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: Nazir Bilal Yavuz <[email protected]>; Alvaro Herrera <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected] Hi, On Thu, Nov 07, 2024 at 04:32:44PM +0000, Bertrand Drouvot wrote: > Hi, > > On Thu, Nov 07, 2024 at 09:50:59AM +0900, Michael Paquier wrote: > > On Wed, Nov 06, 2024 at 01:51:02PM +0000, Bertrand Drouvot wrote: > > > That's not needed, the patch I'm working on stores the proc number in the > > > objid field of the key. > > > > Relying on the procnumber for the object ID gets a +1 here. > > Thanks! > > > That > > provides an automatic cap on the maximum number of entries that can > > exist at once for this new stats kind. > > Yeah. > > POC patch is now working, will wear a reviewer hat now and will share in one > or 2 days. > Please find attached v5 that implements the per-backend IO stats as variable-numbered stats kind (as per the up-thread discussion). It is split into 4 sub-patches: ==== 0001 (the largest one) Introduces a new statistics KIND. It is named PGSTAT_KIND_PER_BACKEND as it could be used in the future to store other statistics (than the I/O ones) per backend. The new KIND is a variable-numbered one and has an automatic cap on the maximum number of entries (as its hash key contains the proc number). There is no need to serialize the per backend I/O stats to disk (no point to see stats for backends that do not exist anymore after a re-start), so a new "to_serialize" field is added in the PgStat_KindInfo struct. It adds a new pg_my_stat_io view to display "my" backend I/O statistics. It also adds a new function pg_stat_reset_single_backend_io_counters() to be able to reset the I/O stats for a given backend pid. It contains doc updates and dedicated tests. A few remarks: 1. one assertion in pgstat_drop_entry_internal() is not necessary true anymore with this new stat kind. So, adding an extra bool as parameter to take care of it. 2. a new struct "PgStat_Backend" is created and does contain the backend type. The backend type is used for filtering purpose when the stats are displayed. 3. when the stats are reset, we need to keep the backend type so the change in shared_stat_reset_contents(). 4. the pending stats are updated in the existing pgstat_count_io_op_n() and pgstat_count_io_op_time() functions. 5. this new kind has its own flush callback: pgstat_per_backend_flush_cb(). But there is no need to maintain 2 callbacks for the I/O stats, so 0002 will merge both and remove the one from the fixed stats. The reason it's not done in 0001 is to ease the review. ==== 0002 Merge both IO stats flush callback. There is no need to keep both callbacks. Merging both allows to save O(N^3) while looping on IOOBJECT_NUM_TYPES, IOCONTEXT_NUM_TYPES and IOCONTEXT_NUM_TYPES. The patch removes: 1. pgstat_io_flush_cb() 2. PendingIOStats (as the same information is already stored in the pending entries) 3. have_iostats (the patch relies on the pending entries instead) ==== 0003 Don't include other backend's stats in the snapshot. When stats_fetch_consistency is set to 'snapshot', don't include other backend's stats in the snapshot. There is no use case, so save memory usage. ==== 0004 Adding the pg_stat_get_backend_io() function to retrieve I/O statistics for a particular backend pid. Note that this function does not return any rows if stats_fetch_consistency is set to 'snapshot' and the pid of interest is not our own pid (there is no use case of retrieving other backend's stats with stats_fetch_consistency set to 'snapshot'). The patch: 1. replaces pg_stat_get_my_io() with pg_stat_get_backend_io() 2. adds pgstat_fetch_proc_stat_io() 3. adds doc 4. adds tests === Remarks R1. The key field is still "objid" even if it stores the proc number. I think that's fine and there is no need to rename it as it's related comment states: " /* object ID (table, function, etc.), or identifier. */ " R2. pg_stat_get_io() and pg_stat_get_backend_io() could probably be merged ( duplicated code). That's not mandatory to provide the new per backend I/O stats feature. So let's keep it as future work to ease the review. R3. There is no use case of retrieving other backend's IO stats with stats_fetch_consistency set to 'snapshot'. Avoiding this behavior allows us to save memory (that could be non negligeable as pgstat_build_snapshot() would add _all_ the other backend's stats in the snapshot). I choose to document it and to not return any rows: I think that's better than returning some data (that would not "ensure" the snapshot consistency) or an error. Looking forward to your feedback, Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com Attachments: [text/x-diff] v5-0001-per-backend-I-O-statistics.patch (40.5K, ../../[email protected]/2-v5-0001-per-backend-I-O-statistics.patch) download | inline diff: From 89de6e71540912ca294be38fea0ed5636b718521 Mon Sep 17 00:00:00 2001 From: Bertrand Drouvot <[email protected]> Date: Mon, 28 Oct 2024 12:50:32 +0000 Subject: [PATCH v5 1/4] per backend I/O statistics While pg_stat_io provides cluster-wide I/O statistics, this commit adds a new statistics kind and a new pg_my_stat_io view to display "my" backend I/O statistics. The new KIND is named PGSTAT_KIND_PER_BACKEND as it could be used in the future to store other statistics (than the I/O ones) per backend. The new KIND is a variable-numbered one and has an automatic cap on the maximum number of entries (as its hash key contains the proc number). There is no need to serialize the per backend I/O stats to disk (no point to see stats for backends that do not exist anymore after a re-start), so a new "to_serialize" field is added in the PgStat_KindInfo struct. Also adding a new function pg_stat_reset_single_backend_io_counters() to be able to reset the I/O stats for a given backend pid. A subsequent commit will add a new pg_stat_get_backend_io() function to be able to retrieve the I/O statistics for a given backend pid. XXX: Bump catalog version needs to be done. --- doc/src/sgml/config.sgml | 4 +- doc/src/sgml/monitoring.sgml | 43 ++++++ src/backend/catalog/system_functions.sql | 2 + src/backend/catalog/system_views.sql | 22 ++++ src/backend/utils/activity/backend_status.c | 3 + src/backend/utils/activity/pgstat.c | 34 ++++- src/backend/utils/activity/pgstat_io.c | 114 ++++++++++++++-- src/backend/utils/activity/pgstat_shmem.c | 30 ++++- src/backend/utils/adt/pgstatfuncs.c | 124 ++++++++++++++++++ src/include/catalog/pg_proc.dat | 14 ++ src/include/pgstat.h | 27 +++- src/include/utils/pgstat_internal.h | 13 ++ .../injection_points/injection_stats.c | 1 + src/test/regress/expected/rules.out | 19 +++ src/test/regress/expected/stats.out | 60 ++++++++- src/test/regress/sql/stats.sql | 31 ++++- src/tools/pgindent/typedefs.list | 2 + 17 files changed, 517 insertions(+), 26 deletions(-) 10.2% doc/src/sgml/ 29.7% src/backend/utils/activity/ 19.2% src/backend/utils/adt/ 5.1% src/include/catalog/ 7.2% src/include/ 14.7% src/test/regress/expected/ 10.4% src/test/regress/sql/ 3.1% src/ diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index d54f904956..fbabf371a6 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -8361,7 +8361,9 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv; displayed in <link linkend="monitoring-pg-stat-database-view"> <structname>pg_stat_database</structname></link>, <link linkend="monitoring-pg-stat-io-view"> - <structname>pg_stat_io</structname></link>, in the output of + <structname>pg_stat_io</structname></link>, + <link linkend="monitoring-pg-my-stat-io-view"> + <structname>pg_my_stat_io</structname></link>, in the output of <xref linkend="sql-explain"/> when the <literal>BUFFERS</literal> option is used, in the output of <xref linkend="sql-vacuum"/> when the <literal>VERBOSE</literal> option is used, by autovacuum diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml index 331315f8d3..fc6aded3da 100644 --- a/doc/src/sgml/monitoring.sgml +++ b/doc/src/sgml/monitoring.sgml @@ -488,6 +488,16 @@ postgres 27093 0.0 0.0 30096 2752 ? Ss 11:34 0:00 postgres: ser </entry> </row> + <row> + <entry><structname>pg_my_stat_io</structname><indexterm><primary>pg_my_stat_io</primary></indexterm></entry> + <entry> + One row for each combination of context and target object containing + my backend I/O statistics. + See <link linkend="monitoring-pg-my-stat-io-view"> + <structname>pg_my_stat_io</structname></link> for details. + </entry> + </row> + <row> <entry><structname>pg_stat_replication_slots</structname><indexterm><primary>pg_stat_replication_slots</primary></indexterm></entry> <entry>One row per replication slot, showing statistics about the @@ -2946,7 +2956,23 @@ description | Waiting for a newly initialized WAL file to reach durable storage </para> </note> + </sect2> + <sect2 id="monitoring-pg-my-stat-io-view"> + <title><structname>pg_my_stat_io</structname></title> + + <indexterm> + <primary>pg_my_stat_io</primary> + </indexterm> + + <para> + The <structname>pg_my_stat_io</structname> view will contain one row for each + combination of target I/O object and I/O context, showing + my backend I/O statistics. Combinations which do not make sense are + omitted. The fields are exactly the same as the ones in the <link + linkend="monitoring-pg-stat-io-view"> <structname>pg_stat_io</structname></link> + view. + </para> </sect2> @@ -4971,6 +4997,23 @@ description | Waiting for a newly initialized WAL file to reach durable storage </para></entry> </row> + <row> + <entry role="func_table_entry"><para role="func_signature"> + <indexterm> + <primary>pg_stat_reset_single_backend_io_counters</primary> + </indexterm> + <function>pg_stat_reset_single_backend_io_counters</function> ( <type>int4</type> ) + <returnvalue>void</returnvalue> + </para> + <para> + Resets I/O statistics for a single backend to zero. + </para> + <para> + This function is restricted to superusers by default, but other users + can be granted EXECUTE to run the function. + </para></entry> + </row> + <row> <entry role="func_table_entry"><para role="func_signature"> <indexterm> diff --git a/src/backend/catalog/system_functions.sql b/src/backend/catalog/system_functions.sql index c51dfca802..8a1ae44fca 100644 --- a/src/backend/catalog/system_functions.sql +++ b/src/backend/catalog/system_functions.sql @@ -711,6 +711,8 @@ REVOKE EXECUTE ON FUNCTION pg_stat_reset_single_table_counters(oid) FROM public; REVOKE EXECUTE ON FUNCTION pg_stat_reset_single_function_counters(oid) FROM public; +REVOKE EXECUTE ON FUNCTION pg_stat_reset_single_backend_io_counters(int4) FROM public; + REVOKE EXECUTE ON FUNCTION pg_stat_reset_replication_slot(text) FROM public; REVOKE EXECUTE ON FUNCTION pg_stat_have_stats(text, oid, int8) FROM public; diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql index 3456b821bc..09af4a40a8 100644 --- a/src/backend/catalog/system_views.sql +++ b/src/backend/catalog/system_views.sql @@ -1170,6 +1170,28 @@ SELECT b.stats_reset FROM pg_stat_get_io() b; +CREATE VIEW pg_my_stat_io AS +SELECT + b.backend_type, + b.object, + b.context, + b.reads, + b.read_time, + b.writes, + b.write_time, + b.writebacks, + b.writeback_time, + b.extends, + b.extend_time, + b.op_bytes, + b.hits, + b.evictions, + b.reuses, + b.fsyncs, + b.fsync_time, + b.stats_reset +FROM pg_stat_get_my_io() b; + CREATE VIEW pg_stat_wal AS SELECT w.wal_records, diff --git a/src/backend/utils/activity/backend_status.c b/src/backend/utils/activity/backend_status.c index bdb3a296ca..aa683e150c 100644 --- a/src/backend/utils/activity/backend_status.c +++ b/src/backend/utils/activity/backend_status.c @@ -249,6 +249,9 @@ pgstat_beinit(void) Assert(MyProcNumber >= 0 && MyProcNumber < NumBackendStatSlots); MyBEEntry = &BackendStatusArray[MyProcNumber]; + /* Create the per backend stat entry */ + pgstat_create_backend_stat(MyProcNumber); + /* Set up a process-exit hook to clean up */ on_shmem_exit(pgstat_beshutdown_hook, 0); } diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c index ea8c5691e8..aacf61c9a4 100644 --- a/src/backend/utils/activity/pgstat.c +++ b/src/backend/utils/activity/pgstat.c @@ -281,6 +281,7 @@ static const PgStat_KindInfo pgstat_kind_builtin_infos[PGSTAT_KIND_BUILTIN_SIZE] .name = "database", .fixed_amount = false, + .to_serialize = true, /* so pg_stat_database entries can be seen in all databases */ .accessed_across_databases = true, @@ -297,6 +298,7 @@ static const PgStat_KindInfo pgstat_kind_builtin_infos[PGSTAT_KIND_BUILTIN_SIZE] .name = "relation", .fixed_amount = false, + .to_serialize = true, .shared_size = sizeof(PgStatShared_Relation), .shared_data_off = offsetof(PgStatShared_Relation, stats), @@ -311,6 +313,7 @@ static const PgStat_KindInfo pgstat_kind_builtin_infos[PGSTAT_KIND_BUILTIN_SIZE] .name = "function", .fixed_amount = false, + .to_serialize = true, .shared_size = sizeof(PgStatShared_Function), .shared_data_off = offsetof(PgStatShared_Function, stats), @@ -324,6 +327,7 @@ static const PgStat_KindInfo pgstat_kind_builtin_infos[PGSTAT_KIND_BUILTIN_SIZE] .name = "replslot", .fixed_amount = false, + .to_serialize = true, .accessed_across_databases = true, @@ -340,6 +344,7 @@ static const PgStat_KindInfo pgstat_kind_builtin_infos[PGSTAT_KIND_BUILTIN_SIZE] .name = "subscription", .fixed_amount = false, + .to_serialize = true, /* so pg_stat_subscription_stats entries can be seen in all databases */ .accessed_across_databases = true, @@ -352,6 +357,22 @@ static const PgStat_KindInfo pgstat_kind_builtin_infos[PGSTAT_KIND_BUILTIN_SIZE] .reset_timestamp_cb = pgstat_subscription_reset_timestamp_cb, }, + [PGSTAT_KIND_PER_BACKEND] = { + .name = "perbackend", + + .fixed_amount = false, + .to_serialize = false, + + .accessed_across_databases = true, + + .shared_size = sizeof(PgStatShared_Backend), + .shared_data_off = offsetof(PgStatShared_Backend, stats), + .shared_data_len = sizeof(((PgStatShared_Backend *) 0)->stats), + .pending_size = sizeof(PgStat_PendingIO), + + .flush_pending_cb = pgstat_per_backend_flush_cb, + .reset_timestamp_cb = pgstat_backend_reset_timestamp_cb, + }, /* stats for fixed-numbered (mostly 1) objects */ @@ -359,6 +380,7 @@ static const PgStat_KindInfo pgstat_kind_builtin_infos[PGSTAT_KIND_BUILTIN_SIZE] .name = "archiver", .fixed_amount = true, + .to_serialize = true, .snapshot_ctl_off = offsetof(PgStat_Snapshot, archiver), .shared_ctl_off = offsetof(PgStat_ShmemControl, archiver), @@ -374,6 +396,7 @@ static const PgStat_KindInfo pgstat_kind_builtin_infos[PGSTAT_KIND_BUILTIN_SIZE] .name = "bgwriter", .fixed_amount = true, + .to_serialize = true, .snapshot_ctl_off = offsetof(PgStat_Snapshot, bgwriter), .shared_ctl_off = offsetof(PgStat_ShmemControl, bgwriter), @@ -389,6 +412,7 @@ static const PgStat_KindInfo pgstat_kind_builtin_infos[PGSTAT_KIND_BUILTIN_SIZE] .name = "checkpointer", .fixed_amount = true, + .to_serialize = true, .snapshot_ctl_off = offsetof(PgStat_Snapshot, checkpointer), .shared_ctl_off = offsetof(PgStat_ShmemControl, checkpointer), @@ -404,6 +428,7 @@ static const PgStat_KindInfo pgstat_kind_builtin_infos[PGSTAT_KIND_BUILTIN_SIZE] .name = "io", .fixed_amount = true, + .to_serialize = true, .snapshot_ctl_off = offsetof(PgStat_Snapshot, io), .shared_ctl_off = offsetof(PgStat_ShmemControl, io), @@ -421,6 +446,7 @@ static const PgStat_KindInfo pgstat_kind_builtin_infos[PGSTAT_KIND_BUILTIN_SIZE] .name = "slru", .fixed_amount = true, + .to_serialize = true, .snapshot_ctl_off = offsetof(PgStat_Snapshot, slru), .shared_ctl_off = offsetof(PgStat_ShmemControl, slru), @@ -438,6 +464,7 @@ static const PgStat_KindInfo pgstat_kind_builtin_infos[PGSTAT_KIND_BUILTIN_SIZE] .name = "wal", .fixed_amount = true, + .to_serialize = true, .snapshot_ctl_off = offsetof(PgStat_Snapshot, wal), .shared_ctl_off = offsetof(PgStat_ShmemControl, wal), @@ -515,7 +542,7 @@ pgstat_discard_stats(void) /* * Reset stats contents. This will set reset timestamps of fixed-numbered - * stats to the current time (no variable stats exist). + * stats to the current time (the per-backend variable stat exists too). */ pgstat_reset_after_failure(); } @@ -756,7 +783,7 @@ pgstat_report_stat(bool force) partial_flush = false; - /* flush database / relation / function / ... stats */ + /* flush database / relation / function / backend / ... stats */ partial_flush |= pgstat_flush_pending_entries(nowait); /* flush of fixed-numbered stats */ @@ -1660,6 +1687,9 @@ pgstat_write_statsfile(XLogRecPtr redo) kind_info = pgstat_get_kind_info(ps->key.kind); + if (!kind_info->to_serialize) + continue; + /* if not dropped the valid-entry refcount should exist */ Assert(pg_atomic_read_u32(&ps->refcount) > 0); diff --git a/src/backend/utils/activity/pgstat_io.c b/src/backend/utils/activity/pgstat_io.c index cc2ffc78aa..a2bb11adc9 100644 --- a/src/backend/utils/activity/pgstat_io.c +++ b/src/backend/utils/activity/pgstat_io.c @@ -21,13 +21,6 @@ #include "utils/pgstat_internal.h" -typedef struct PgStat_PendingIO -{ - PgStat_Counter counts[IOOBJECT_NUM_TYPES][IOCONTEXT_NUM_TYPES][IOOP_NUM_TYPES]; - instr_time pending_times[IOOBJECT_NUM_TYPES][IOCONTEXT_NUM_TYPES][IOOP_NUM_TYPES]; -} PgStat_PendingIO; - - static PgStat_PendingIO PendingIOStats; static bool have_iostats = false; @@ -82,12 +75,17 @@ pgstat_count_io_op(IOObject io_object, IOContext io_context, IOOp io_op) void pgstat_count_io_op_n(IOObject io_object, IOContext io_context, IOOp io_op, uint32 cnt) { + PgStat_PendingIO *entry_ref; + Assert((unsigned int) io_object < IOOBJECT_NUM_TYPES); Assert((unsigned int) io_context < IOCONTEXT_NUM_TYPES); Assert((unsigned int) io_op < IOOP_NUM_TYPES); Assert(pgstat_tracks_io_op(MyBackendType, io_object, io_context, io_op)); + entry_ref = pgstat_prep_per_backend_pending(MyProcNumber); + PendingIOStats.counts[io_object][io_context][io_op] += cnt; + entry_ref->counts[io_object][io_context][io_op] += cnt; have_iostats = true; } @@ -122,6 +120,10 @@ void pgstat_count_io_op_time(IOObject io_object, IOContext io_context, IOOp io_op, instr_time start_time, uint32 cnt) { + PgStat_PendingIO *entry_ref; + + entry_ref = pgstat_prep_per_backend_pending(MyProcNumber); + if (track_io_timing) { instr_time io_time; @@ -148,6 +150,8 @@ pgstat_count_io_op_time(IOObject io_object, IOContext io_context, IOOp io_op, INSTR_TIME_ADD(PendingIOStats.pending_times[io_object][io_context][io_op], io_time); + INSTR_TIME_ADD(entry_ref->pending_times[io_object][io_context][io_op], + io_time); } pgstat_count_io_op_n(io_object, io_context, io_op, cnt); @@ -161,6 +165,13 @@ pgstat_fetch_stat_io(void) return &pgStatLocal.snapshot.io; } +PgStat_Backend * +pgstat_fetch_my_stat_io(void) +{ + return (PgStat_Backend *) + pgstat_fetch_entry(PGSTAT_KIND_PER_BACKEND, InvalidOid, MyProcNumber); +} + /* * Check if there any IO stats waiting for flush. */ @@ -171,12 +182,18 @@ pgstat_io_have_pending_cb(void) } /* - * Simpler wrapper of pgstat_io_flush_cb() + * Simpler wrapper of pgstat_io_flush_cb() and pgstat_per_backend_flush_cb(). */ void pgstat_flush_io(bool nowait) { + PgStat_EntryRef *entry_ref; + + entry_ref = pgstat_get_entry_ref(PGSTAT_KIND_PER_BACKEND, InvalidOid, + MyProcNumber, false, NULL); + (void) pgstat_io_flush_cb(nowait); + (void) pgstat_per_backend_flush_cb(entry_ref, nowait); } /* @@ -235,6 +252,49 @@ pgstat_io_flush_cb(bool nowait) return false; } +/* + * Flush out locally pending backend statistics + * + * If no stats have been recorded, this function returns false. + */ +bool +pgstat_per_backend_flush_cb(PgStat_EntryRef *entry_ref, bool nowait) +{ + PgStatShared_Backend *shbackendioent; + PgStat_PendingIO *pendingent; + PgStat_BktypeIO *bktype_shstats; + + if (!pgstat_lock_entry(entry_ref, nowait)) + return false; + + shbackendioent = (PgStatShared_Backend *) entry_ref->shared_stats; + bktype_shstats = &shbackendioent->stats.stats; + pendingent = (PgStat_PendingIO *) entry_ref->pending; + + for (int io_object = 0; io_object < IOOBJECT_NUM_TYPES; io_object++) + { + for (int io_context = 0; io_context < IOCONTEXT_NUM_TYPES; io_context++) + { + for (int io_op = 0; io_op < IOOP_NUM_TYPES; io_op++) + { + instr_time time; + + bktype_shstats->counts[io_object][io_context][io_op] += + pendingent->counts[io_object][io_context][io_op]; + + time = pendingent->pending_times[io_object][io_context][io_op]; + + bktype_shstats->times[io_object][io_context][io_op] += + INSTR_TIME_GET_MICROSEC(time); + } + } + } + + pgstat_unlock_entry(entry_ref); + + return true; +} + const char * pgstat_get_io_context_name(IOContext io_context) { @@ -325,6 +385,38 @@ pgstat_io_snapshot_cb(void) } } +void +pgstat_create_backend_stat(ProcNumber procnum) +{ + PgStat_EntryRef *entry_ref; + PgStatShared_Backend *shstatent; + + entry_ref = pgstat_prep_pending_entry(PGSTAT_KIND_PER_BACKEND, InvalidOid, + procnum, NULL); + + shstatent = (PgStatShared_Backend *) entry_ref->shared_stats; + + /* + * NB: need to accept that there might be stats from an older backend, + * e.g. if we previously used this proc number. + */ + memset(&shstatent->stats, 0, sizeof(shstatent->stats)); + + /* set the backend type */ + shstatent->stats.bktype = MyBackendType; +} + +PgStat_PendingIO * +pgstat_prep_per_backend_pending(ProcNumber procnum) +{ + PgStat_EntryRef *entry_ref; + + entry_ref = pgstat_prep_pending_entry(PGSTAT_KIND_PER_BACKEND, InvalidOid, + procnum, NULL); + + return entry_ref->pending; +} + /* * IO statistics are not collected for all BackendTypes. * @@ -503,3 +595,9 @@ pgstat_tracks_io_op(BackendType bktype, IOObject io_object, return true; } + +void +pgstat_backend_reset_timestamp_cb(PgStatShared_Common *header, TimestampTz ts) +{ + ((PgStatShared_Backend *) header)->stats.stat_reset_timestamp = ts; +} diff --git a/src/backend/utils/activity/pgstat_shmem.c b/src/backend/utils/activity/pgstat_shmem.c index c1b7ff76b1..b871c1d7d0 100644 --- a/src/backend/utils/activity/pgstat_shmem.c +++ b/src/backend/utils/activity/pgstat_shmem.c @@ -813,13 +813,24 @@ pgstat_free_entry(PgStatShared_HashEntry *shent, dshash_seq_status *hstat) */ static bool pgstat_drop_entry_internal(PgStatShared_HashEntry *shent, - dshash_seq_status *hstat) + dshash_seq_status *hstat, bool ignore_aux_proc) { Assert(shent->body != InvalidDsaPointer); /* should already have released local reference */ if (pgStatEntryRefHash) - Assert(!pgstat_entry_ref_hash_lookup(pgStatEntryRefHash, shent->key)); + { + /* + * The following assertion is not correct when coming from + * pgstat_discard_stats() and for per backend stats linked to an + * auxiliary process. + */ + if (ignore_aux_proc && shent->key.kind == PGSTAT_KIND_PER_BACKEND && + MyProcNumber >= MaxBackends) + return false; + else + Assert(!pgstat_entry_ref_hash_lookup(pgStatEntryRefHash, shent->key)); + } /* * Signal that the entry is dropped - this will eventually cause other @@ -882,7 +893,7 @@ pgstat_drop_database_and_contents(Oid dboid) if (p->key.dboid != dboid) continue; - if (!pgstat_drop_entry_internal(p, &hstat)) + if (!pgstat_drop_entry_internal(p, &hstat, false)) { /* * Even statistics for a dropped database might currently be @@ -941,7 +952,7 @@ pgstat_drop_entry(PgStat_Kind kind, Oid dboid, uint64 objid) shent = dshash_find(pgStatLocal.shared_hash, &key, true); if (shent) { - freed = pgstat_drop_entry_internal(shent, NULL); + freed = pgstat_drop_entry_internal(shent, NULL, false); /* * Database stats contain other stats. Drop those as well when @@ -969,7 +980,7 @@ pgstat_drop_all_entries(void) if (ps->dropped) continue; - if (!pgstat_drop_entry_internal(ps, &hstat)) + if (!pgstat_drop_entry_internal(ps, &hstat, true)) not_freed_count++; } dshash_seq_term(&hstat); @@ -982,11 +993,20 @@ static void shared_stat_reset_contents(PgStat_Kind kind, PgStatShared_Common *header, TimestampTz ts) { + BackendType bktype; const PgStat_KindInfo *kind_info = pgstat_get_kind_info(kind); + /* save the bktype */ + if (kind == PGSTAT_KIND_PER_BACKEND) + bktype = ((PgStatShared_Backend *) header)->stats.bktype; + memset(pgstat_get_entry_data(kind, header), 0, pgstat_get_entry_len(kind)); + /* restore the bktype */ + if (kind == PGSTAT_KIND_PER_BACKEND) + ((PgStatShared_Backend *) header)->stats.bktype = bktype; + if (kind_info->reset_timestamp_cb) kind_info->reset_timestamp_cb(header, ts); } diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c index f7b50e0b5a..fd16986156 100644 --- a/src/backend/utils/adt/pgstatfuncs.c +++ b/src/backend/utils/adt/pgstatfuncs.c @@ -1474,6 +1474,111 @@ pg_stat_get_io(PG_FUNCTION_ARGS) return (Datum) 0; } +Datum +pg_stat_get_my_io(PG_FUNCTION_ARGS) +{ + ReturnSetInfo *rsinfo; + PgStat_Backend *backend_stats; + Datum bktype_desc; + PgStat_BktypeIO *bktype_stats; + BackendType bktype; + Datum reset_time; + + InitMaterializedSRF(fcinfo, 0); + rsinfo = (ReturnSetInfo *) fcinfo->resultinfo; + + backend_stats = pgstat_fetch_my_stat_io(); + + bktype = backend_stats->bktype; + bktype_desc = CStringGetTextDatum(GetBackendTypeDesc(bktype)); + bktype_stats = &backend_stats->stats; + reset_time = TimestampTzGetDatum(backend_stats->stat_reset_timestamp); + + /* + * In Assert builds, we can afford an extra loop through all of the + * counters checking that only expected stats are non-zero, since it keeps + * the non-Assert code cleaner. + */ + Assert(pgstat_bktype_io_stats_valid(bktype_stats, bktype)); + + for (int io_obj = 0; io_obj < IOOBJECT_NUM_TYPES; io_obj++) + { + const char *obj_name = pgstat_get_io_object_name(io_obj); + + for (int io_context = 0; io_context < IOCONTEXT_NUM_TYPES; io_context++) + { + const char *context_name = pgstat_get_io_context_name(io_context); + + Datum values[IO_NUM_COLUMNS] = {0}; + bool nulls[IO_NUM_COLUMNS] = {0}; + + /* + * Some combinations of BackendType, IOObject, and IOContext are + * not valid for any type of IOOp. In such cases, omit the entire + * row from the view. + */ + if (!pgstat_tracks_io_object(bktype, io_obj, io_context)) + continue; + + values[IO_COL_BACKEND_TYPE] = bktype_desc; + values[IO_COL_CONTEXT] = CStringGetTextDatum(context_name); + values[IO_COL_OBJECT] = CStringGetTextDatum(obj_name); + if (backend_stats->stat_reset_timestamp != 0) + values[IO_COL_RESET_TIME] = reset_time; + else + nulls[IO_COL_RESET_TIME] = true; + + /* + * Hard-code this to the value of BLCKSZ for now. Future values + * could include XLOG_BLCKSZ, once WAL IO is tracked, and constant + * multipliers, once non-block-oriented IO (e.g. temporary file + * IO) is tracked. + */ + values[IO_COL_CONVERSION] = Int64GetDatum(BLCKSZ); + + for (int io_op = 0; io_op < IOOP_NUM_TYPES; io_op++) + { + int op_idx = pgstat_get_io_op_index(io_op); + int time_idx = pgstat_get_io_time_index(io_op); + + /* + * Some combinations of BackendType and IOOp, of IOContext and + * IOOp, and of IOObject and IOOp are not tracked. Set these + * cells in the view NULL. + */ + if (pgstat_tracks_io_op(bktype, io_obj, io_context, io_op)) + { + PgStat_Counter count = + bktype_stats->counts[io_obj][io_context][io_op]; + + values[op_idx] = Int64GetDatum(count); + } + else + nulls[op_idx] = true; + + /* not every operation is timed */ + if (time_idx == IO_COL_INVALID) + continue; + + if (!nulls[op_idx]) + { + PgStat_Counter time = + bktype_stats->times[io_obj][io_context][io_op]; + + values[time_idx] = Float8GetDatum(pg_stat_us_to_ms(time)); + } + else + nulls[time_idx] = true; + } + + tuplestore_putvalues(rsinfo->setResult, rsinfo->setDesc, + values, nulls); + } + } + + return (Datum) 0; +} + /* * Returns statistics of WAL activity */ @@ -1722,6 +1827,7 @@ pg_stat_reset_shared(PG_FUNCTION_ARGS) pgstat_reset_of_kind(PGSTAT_KIND_BGWRITER); pgstat_reset_of_kind(PGSTAT_KIND_CHECKPOINTER); pgstat_reset_of_kind(PGSTAT_KIND_IO); + pgstat_reset_of_kind(PGSTAT_KIND_PER_BACKEND); XLogPrefetchResetStats(); pgstat_reset_of_kind(PGSTAT_KIND_SLRU); pgstat_reset_of_kind(PGSTAT_KIND_WAL); @@ -1779,6 +1885,24 @@ pg_stat_reset_single_function_counters(PG_FUNCTION_ARGS) PG_RETURN_VOID(); } +Datum +pg_stat_reset_single_backend_io_counters(PG_FUNCTION_ARGS) +{ + PGPROC *proc; + int backend_pid = PG_GETARG_INT32(0); + + proc = BackendPidGetProc(backend_pid); + + /* Maybe an auxiliary process? */ + if (proc == NULL) + proc = AuxiliaryPidGetProc(backend_pid); + + if (proc) + pgstat_reset(PGSTAT_KIND_PER_BACKEND, InvalidOid, GetNumberFromPGProc(proc)); + + PG_RETURN_VOID(); +} + /* Reset SLRU counters (a specific one or all of them). */ Datum pg_stat_reset_slru(PG_FUNCTION_ARGS) diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat index f23321a41f..89eb89efe4 100644 --- a/src/include/catalog/pg_proc.dat +++ b/src/include/catalog/pg_proc.dat @@ -5903,6 +5903,15 @@ proargnames => '{backend_type,object,context,reads,read_time,writes,write_time,writebacks,writeback_time,extends,extend_time,op_bytes,hits,evictions,reuses,fsyncs,fsync_time,stats_reset}', prosrc => 'pg_stat_get_io' }, +{ oid => '8806', descr => 'statistics: my backend IO statistics', + proname => 'pg_stat_get_my_io', prorows => '5', proretset => 't', + provolatile => 'v', proparallel => 'r', prorettype => 'record', + proargtypes => '', + proallargtypes => '{text,text,text,int8,float8,int8,float8,int8,float8,int8,float8,int8,int8,int8,int8,int8,float8,timestamptz}', + proargmodes => '{o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o}', + proargnames => '{backend_type,object,context,reads,read_time,writes,write_time,writebacks,writeback_time,extends,extend_time,op_bytes,hits,evictions,reuses,fsyncs,fsync_time,stats_reset}', + prosrc => 'pg_stat_get_my_io' }, + { oid => '1136', descr => 'statistics: information about WAL activity', proname => 'pg_stat_get_wal', proisstrict => 'f', provolatile => 's', proparallel => 'r', prorettype => 'record', proargtypes => '', @@ -6042,6 +6051,11 @@ proname => 'pg_stat_reset_single_function_counters', provolatile => 'v', prorettype => 'void', proargtypes => 'oid', prosrc => 'pg_stat_reset_single_function_counters' }, +{ oid => '9987', + descr => 'statistics: reset collected IO statistics for a single backend', + proname => 'pg_stat_reset_single_backend_io_counters', provolatile => 'v', + prorettype => 'void', proargtypes => 'int4', + prosrc => 'pg_stat_reset_single_backend_io_counters' }, { oid => '2307', descr => 'statistics: reset collected statistics for a single SLRU', proname => 'pg_stat_reset_slru', proisstrict => 'f', provolatile => 'v', diff --git a/src/include/pgstat.h b/src/include/pgstat.h index df53fa2d4f..13b9c71759 100644 --- a/src/include/pgstat.h +++ b/src/include/pgstat.h @@ -49,14 +49,15 @@ #define PGSTAT_KIND_FUNCTION 3 /* per-function statistics */ #define PGSTAT_KIND_REPLSLOT 4 /* per-slot statistics */ #define PGSTAT_KIND_SUBSCRIPTION 5 /* per-subscription statistics */ +#define PGSTAT_KIND_PER_BACKEND 6 /* stats for fixed-numbered objects */ -#define PGSTAT_KIND_ARCHIVER 6 -#define PGSTAT_KIND_BGWRITER 7 -#define PGSTAT_KIND_CHECKPOINTER 8 -#define PGSTAT_KIND_IO 9 -#define PGSTAT_KIND_SLRU 10 -#define PGSTAT_KIND_WAL 11 +#define PGSTAT_KIND_ARCHIVER 7 +#define PGSTAT_KIND_BGWRITER 8 +#define PGSTAT_KIND_CHECKPOINTER 9 +#define PGSTAT_KIND_IO 10 +#define PGSTAT_KIND_SLRU 11 +#define PGSTAT_KIND_WAL 12 #define PGSTAT_KIND_BUILTIN_MIN PGSTAT_KIND_DATABASE #define PGSTAT_KIND_BUILTIN_MAX PGSTAT_KIND_WAL @@ -347,12 +348,24 @@ typedef struct PgStat_BktypeIO PgStat_Counter times[IOOBJECT_NUM_TYPES][IOCONTEXT_NUM_TYPES][IOOP_NUM_TYPES]; } PgStat_BktypeIO; +typedef struct PgStat_PendingIO +{ + PgStat_Counter counts[IOOBJECT_NUM_TYPES][IOCONTEXT_NUM_TYPES][IOOP_NUM_TYPES]; + instr_time pending_times[IOOBJECT_NUM_TYPES][IOCONTEXT_NUM_TYPES][IOOP_NUM_TYPES]; +} PgStat_PendingIO; + typedef struct PgStat_IO { TimestampTz stat_reset_timestamp; PgStat_BktypeIO stats[BACKEND_NUM_TYPES]; } PgStat_IO; +typedef struct PgStat_Backend +{ + TimestampTz stat_reset_timestamp; + BackendType bktype; + PgStat_BktypeIO stats; +} PgStat_Backend; typedef struct PgStat_StatDBEntry { @@ -562,6 +575,7 @@ extern void pgstat_count_io_op_time(IOObject io_object, IOContext io_context, IOOp io_op, instr_time start_time, uint32 cnt); extern PgStat_IO *pgstat_fetch_stat_io(void); +extern PgStat_Backend *pgstat_fetch_my_stat_io(void); extern const char *pgstat_get_io_context_name(IOContext io_context); extern const char *pgstat_get_io_object_name(IOObject io_object); @@ -570,6 +584,7 @@ extern bool pgstat_tracks_io_object(BackendType bktype, IOObject io_object, IOContext io_context); extern bool pgstat_tracks_io_op(BackendType bktype, IOObject io_object, IOContext io_context, IOOp io_op); +extern void pgstat_create_backend_stat(ProcNumber procnum); /* diff --git a/src/include/utils/pgstat_internal.h b/src/include/utils/pgstat_internal.h index 61b2e1f96b..44a00d4779 100644 --- a/src/include/utils/pgstat_internal.h +++ b/src/include/utils/pgstat_internal.h @@ -194,6 +194,11 @@ typedef struct PgStat_KindInfo */ bool accessed_across_databases:1; + /* + * Do serialize or not this kind of stats. + */ + bool to_serialize:1; + /* * The size of an entry in the shared stats hash table (pointed to by * PgStatShared_HashEntry->body). For fixed-numbered statistics, this is @@ -428,6 +433,11 @@ typedef struct PgStatShared_ReplSlot PgStat_StatReplSlotEntry stats; } PgStatShared_ReplSlot; +typedef struct PgStatShared_Backend +{ + PgStatShared_Common header; + PgStat_Backend stats; +} PgStatShared_Backend; /* * Central shared memory entry for the cumulative stats system. @@ -630,9 +640,12 @@ extern void pgstat_flush_io(bool nowait); extern bool pgstat_io_have_pending_cb(void); extern bool pgstat_io_flush_cb(bool nowait); +extern bool pgstat_per_backend_flush_cb(PgStat_EntryRef *entry_ref, 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); +extern PgStat_PendingIO *pgstat_prep_per_backend_pending(ProcNumber procnum); +extern void pgstat_backend_reset_timestamp_cb(PgStatShared_Common *header, TimestampTz ts); /* diff --git a/src/test/modules/injection_points/injection_stats.c b/src/test/modules/injection_points/injection_stats.c index d89d055913..ca4b6b6e9f 100644 --- a/src/test/modules/injection_points/injection_stats.c +++ b/src/test/modules/injection_points/injection_stats.c @@ -39,6 +39,7 @@ static bool injection_stats_flush_cb(PgStat_EntryRef *entry_ref, bool nowait); static const PgStat_KindInfo injection_stats = { .name = "injection_points", .fixed_amount = false, /* Bounded by the number of points */ + .to_serialize = true, /* Injection points are system-wide */ .accessed_across_databases = true, diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out index 2b47013f11..f92516b047 100644 --- a/src/test/regress/expected/rules.out +++ b/src/test/regress/expected/rules.out @@ -1398,6 +1398,25 @@ pg_matviews| SELECT n.nspname AS schemaname, LEFT JOIN pg_namespace n ON ((n.oid = c.relnamespace))) LEFT JOIN pg_tablespace t ON ((t.oid = c.reltablespace))) WHERE (c.relkind = 'm'::"char"); +pg_my_stat_io| SELECT backend_type, + object, + context, + reads, + read_time, + writes, + write_time, + writebacks, + writeback_time, + extends, + extend_time, + op_bytes, + hits, + evictions, + reuses, + fsyncs, + fsync_time, + stats_reset + FROM pg_stat_get_my_io() b(backend_type, object, context, reads, read_time, writes, write_time, writebacks, writeback_time, extends, extend_time, op_bytes, hits, evictions, reuses, fsyncs, fsync_time, stats_reset); pg_policies| SELECT n.nspname AS schemaname, c.relname AS tablename, pol.polname AS policyname, diff --git a/src/test/regress/expected/stats.out b/src/test/regress/expected/stats.out index 56771f83ed..846e693483 100644 --- a/src/test/regress/expected/stats.out +++ b/src/test/regress/expected/stats.out @@ -1249,7 +1249,7 @@ SELECT pg_stat_get_subscription_stats(NULL); (1 row) --- Test that the following operations are tracked in pg_stat_io: +-- Test that the following operations are tracked in pg_[my_]stat_io: -- - reads of target blocks into shared buffers -- - writes of shared buffers to permanent storage -- - extends of relations using shared buffers @@ -1261,9 +1261,14 @@ SELECT pg_stat_get_subscription_stats(NULL); -- extends. SELECT sum(extends) AS io_sum_shared_before_extends FROM pg_stat_io WHERE context = 'normal' AND object = 'relation' \gset +SELECT sum(extends) AS my_io_sum_shared_before_extends + FROM pg_my_stat_io WHERE context = 'normal' AND object = 'relation' \gset SELECT sum(writes) AS writes, sum(fsyncs) AS fsyncs FROM pg_stat_io WHERE object = 'relation' \gset io_sum_shared_before_ +SELECT sum(writes) AS writes, sum(fsyncs) AS fsyncs + FROM pg_my_stat_io + WHERE object = 'relation' \gset my_io_sum_shared_before_ CREATE TABLE test_io_shared(a int); INSERT INTO test_io_shared SELECT i FROM generate_series(1,100)i; SELECT pg_stat_force_next_flush(); @@ -1280,8 +1285,16 @@ SELECT :io_sum_shared_after_extends > :io_sum_shared_before_extends; t (1 row) +SELECT sum(extends) AS my_io_sum_shared_after_extends + FROM pg_my_stat_io WHERE context = 'normal' AND object = 'relation' \gset +SELECT :my_io_sum_shared_after_extends > :my_io_sum_shared_before_extends; + ?column? +---------- + t +(1 row) + -- After a checkpoint, there should be some additional IOCONTEXT_NORMAL writes --- and fsyncs. +-- and fsyncs in the global stats (not for the backend). -- See comment above for rationale for two explicit CHECKPOINTs. CHECKPOINT; CHECKPOINT; @@ -1301,6 +1314,23 @@ SELECT current_setting('fsync') = 'off' t (1 row) +SELECT sum(writes) AS writes, sum(fsyncs) AS fsyncs + FROM pg_my_stat_io + WHERE object = 'relation' \gset my_io_sum_shared_after_ +SELECT :my_io_sum_shared_after_writes >= :my_io_sum_shared_before_writes; + ?column? +---------- + t +(1 row) + +SELECT current_setting('fsync') = 'off' + OR (:my_io_sum_shared_after_fsyncs = :my_io_sum_shared_before_fsyncs + AND :my_io_sum_shared_after_fsyncs= 0); + ?column? +---------- + t +(1 row) + -- Change the tablespace so that the table is rewritten directly, then SELECT -- from it to cause it to be read back into shared buffers. SELECT sum(reads) AS io_sum_shared_before_reads @@ -1521,6 +1551,8 @@ SELECT pg_stat_have_stats('io', 0, 0); SELECT sum(evictions) + sum(reuses) + sum(extends) + sum(fsyncs) + sum(reads) + sum(writes) + sum(writebacks) + sum(hits) AS io_stats_pre_reset FROM pg_stat_io \gset +SELECT sum(evictions) + sum(reuses) + sum(extends) + sum(fsyncs) + sum(reads) + sum(writes) + sum(writebacks) + sum(hits) AS my_io_stats_pre_reset + FROM pg_my_stat_io \gset SELECT pg_stat_reset_shared('io'); pg_stat_reset_shared ---------------------- @@ -1535,6 +1567,30 @@ SELECT :io_stats_post_reset < :io_stats_pre_reset; t (1 row) +SELECT sum(evictions) + sum(reuses) + sum(extends) + sum(fsyncs) + sum(reads) + sum(writes) + sum(writebacks) + sum(hits) AS my_io_stats_post_reset + FROM pg_my_stat_io \gset +-- pg_stat_reset_shared() did not reset backend IO stats +SELECT :my_io_stats_pre_reset <= :my_io_stats_post_reset; + ?column? +---------- + t +(1 row) + +-- but pg_stat_reset_single_backend_io_counters() does +SELECT pg_stat_reset_single_backend_io_counters(pg_backend_pid()); + pg_stat_reset_single_backend_io_counters +------------------------------------------ + +(1 row) + +SELECT sum(evictions) + sum(reuses) + sum(extends) + sum(fsyncs) + sum(reads) + sum(writes) + sum(writebacks) + sum(hits) AS my_io_stats_post_backend_reset + FROM pg_my_stat_io \gset +SELECT :my_io_stats_pre_reset > :my_io_stats_post_backend_reset; + ?column? +---------- + t +(1 row) + -- test BRIN index doesn't block HOT update CREATE TABLE brin_hot ( id integer PRIMARY KEY, diff --git a/src/test/regress/sql/stats.sql b/src/test/regress/sql/stats.sql index 7147cc2f89..9cb14b7182 100644 --- a/src/test/regress/sql/stats.sql +++ b/src/test/regress/sql/stats.sql @@ -595,7 +595,7 @@ SELECT pg_stat_get_replication_slot(NULL); SELECT pg_stat_get_subscription_stats(NULL); --- Test that the following operations are tracked in pg_stat_io: +-- Test that the following operations are tracked in pg_[my_]stat_io: -- - reads of target blocks into shared buffers -- - writes of shared buffers to permanent storage -- - extends of relations using shared buffers @@ -609,18 +609,26 @@ SELECT pg_stat_get_subscription_stats(NULL); -- extends. SELECT sum(extends) AS io_sum_shared_before_extends FROM pg_stat_io WHERE context = 'normal' AND object = 'relation' \gset +SELECT sum(extends) AS my_io_sum_shared_before_extends + FROM pg_my_stat_io WHERE context = 'normal' AND object = 'relation' \gset SELECT sum(writes) AS writes, sum(fsyncs) AS fsyncs FROM pg_stat_io WHERE object = 'relation' \gset io_sum_shared_before_ +SELECT sum(writes) AS writes, sum(fsyncs) AS fsyncs + FROM pg_my_stat_io + WHERE object = 'relation' \gset my_io_sum_shared_before_ CREATE TABLE test_io_shared(a int); INSERT INTO test_io_shared SELECT i FROM generate_series(1,100)i; SELECT pg_stat_force_next_flush(); SELECT sum(extends) AS io_sum_shared_after_extends FROM pg_stat_io WHERE context = 'normal' AND object = 'relation' \gset SELECT :io_sum_shared_after_extends > :io_sum_shared_before_extends; +SELECT sum(extends) AS my_io_sum_shared_after_extends + FROM pg_my_stat_io WHERE context = 'normal' AND object = 'relation' \gset +SELECT :my_io_sum_shared_after_extends > :my_io_sum_shared_before_extends; -- After a checkpoint, there should be some additional IOCONTEXT_NORMAL writes --- and fsyncs. +-- and fsyncs in the global stats (not for the backend). -- See comment above for rationale for two explicit CHECKPOINTs. CHECKPOINT; CHECKPOINT; @@ -631,6 +639,14 @@ SELECT :io_sum_shared_after_writes > :io_sum_shared_before_writes; SELECT current_setting('fsync') = 'off' OR :io_sum_shared_after_fsyncs > :io_sum_shared_before_fsyncs; +SELECT sum(writes) AS writes, sum(fsyncs) AS fsyncs + FROM pg_my_stat_io + WHERE object = 'relation' \gset my_io_sum_shared_after_ +SELECT :my_io_sum_shared_after_writes >= :my_io_sum_shared_before_writes; +SELECT current_setting('fsync') = 'off' + OR (:my_io_sum_shared_after_fsyncs = :my_io_sum_shared_before_fsyncs + AND :my_io_sum_shared_after_fsyncs= 0); + -- Change the tablespace so that the table is rewritten directly, then SELECT -- from it to cause it to be read back into shared buffers. SELECT sum(reads) AS io_sum_shared_before_reads @@ -762,10 +778,21 @@ SELECT :io_sum_bulkwrite_strategy_extends_after > :io_sum_bulkwrite_strategy_ext SELECT pg_stat_have_stats('io', 0, 0); SELECT sum(evictions) + sum(reuses) + sum(extends) + sum(fsyncs) + sum(reads) + sum(writes) + sum(writebacks) + sum(hits) AS io_stats_pre_reset FROM pg_stat_io \gset +SELECT sum(evictions) + sum(reuses) + sum(extends) + sum(fsyncs) + sum(reads) + sum(writes) + sum(writebacks) + sum(hits) AS my_io_stats_pre_reset + FROM pg_my_stat_io \gset SELECT pg_stat_reset_shared('io'); SELECT sum(evictions) + sum(reuses) + sum(extends) + sum(fsyncs) + sum(reads) + sum(writes) + sum(writebacks) + sum(hits) AS io_stats_post_reset FROM pg_stat_io \gset SELECT :io_stats_post_reset < :io_stats_pre_reset; +SELECT sum(evictions) + sum(reuses) + sum(extends) + sum(fsyncs) + sum(reads) + sum(writes) + sum(writebacks) + sum(hits) AS my_io_stats_post_reset + FROM pg_my_stat_io \gset +-- pg_stat_reset_shared() did not reset backend IO stats +SELECT :my_io_stats_pre_reset <= :my_io_stats_post_reset; +-- but pg_stat_reset_single_backend_io_counters() does +SELECT pg_stat_reset_single_backend_io_counters(pg_backend_pid()); +SELECT sum(evictions) + sum(reuses) + sum(extends) + sum(fsyncs) + sum(reads) + sum(writes) + sum(writebacks) + sum(hits) AS my_io_stats_post_backend_reset + FROM pg_my_stat_io \gset +SELECT :my_io_stats_pre_reset > :my_io_stats_post_backend_reset; -- test BRIN index doesn't block HOT update diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list index 1847bbfa95..4188d056ca 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -2117,6 +2117,7 @@ PgFdwSamplingMethod PgFdwScanState PgIfAddrCallback PgStatShared_Archiver +PgStatShared_Backend PgStatShared_BgWriter PgStatShared_Checkpointer PgStatShared_Common @@ -2132,6 +2133,7 @@ PgStatShared_SLRU PgStatShared_Subscription PgStatShared_Wal PgStat_ArchiverStats +PgStat_Backend PgStat_BackendSubEntry PgStat_BgWriterStats PgStat_BktypeIO -- 2.34.1 [text/x-diff] v5-0002-Merge-both-IO-stats-flush-callbacks.patch (6.3K, ../../[email protected]/3-v5-0002-Merge-both-IO-stats-flush-callbacks.patch) download | inline diff: From edd45c3aa0731a9645dbcfe0475e40c642e92269 Mon Sep 17 00:00:00 2001 From: Bertrand Drouvot <[email protected]> Date: Wed, 6 Nov 2024 16:19:44 +0000 Subject: [PATCH v5 2/4] Merge both IO stats flush callbacks There is no need to keep both callbacks. Merging both allows to save O(N^3) while looping on IOOBJECT_NUM_TYPES, IOCONTEXT_NUM_TYPES and IOCONTEXT_NUM_TYPES. --- src/backend/utils/activity/pgstat.c | 2 - src/backend/utils/activity/pgstat_io.c | 97 ++++++-------------------- 2 files changed, 23 insertions(+), 76 deletions(-) 100.0% src/backend/utils/activity/ diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c index aacf61c9a4..d051db5c10 100644 --- a/src/backend/utils/activity/pgstat.c +++ b/src/backend/utils/activity/pgstat.c @@ -435,8 +435,6 @@ 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, diff --git a/src/backend/utils/activity/pgstat_io.c b/src/backend/utils/activity/pgstat_io.c index a2bb11adc9..1384f8103e 100644 --- a/src/backend/utils/activity/pgstat_io.c +++ b/src/backend/utils/activity/pgstat_io.c @@ -21,10 +21,6 @@ #include "utils/pgstat_internal.h" -static PgStat_PendingIO PendingIOStats; -static bool have_iostats = false; - - /* * Check that stats have not been counted for any combination of IOObject, * IOContext, and IOOp which are not tracked for the passed-in BackendType. If @@ -83,11 +79,7 @@ pgstat_count_io_op_n(IOObject io_object, IOContext io_context, IOOp io_op, uint3 Assert(pgstat_tracks_io_op(MyBackendType, io_object, io_context, io_op)); entry_ref = pgstat_prep_per_backend_pending(MyProcNumber); - - PendingIOStats.counts[io_object][io_context][io_op] += cnt; entry_ref->counts[io_object][io_context][io_op] += cnt; - - have_iostats = true; } /* @@ -148,8 +140,6 @@ pgstat_count_io_op_time(IOObject io_object, IOContext io_context, IOOp io_op, INSTR_TIME_ADD(pgBufferUsage.local_blk_read_time, io_time); } - INSTR_TIME_ADD(PendingIOStats.pending_times[io_object][io_context][io_op], - io_time); INSTR_TIME_ADD(entry_ref->pending_times[io_object][io_context][io_op], io_time); } @@ -173,16 +163,7 @@ pgstat_fetch_my_stat_io(void) } /* - * 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() and pgstat_per_backend_flush_cb(). + * Simpler wrapper of pgstat_per_backend_flush_cb(). */ void pgstat_flush_io(bool nowait) @@ -192,81 +173,39 @@ pgstat_flush_io(bool nowait) entry_ref = pgstat_get_entry_ref(PGSTAT_KIND_PER_BACKEND, InvalidOid, MyProcNumber, false, NULL); - (void) pgstat_io_flush_cb(nowait); (void) pgstat_per_backend_flush_cb(entry_ref, nowait); } /* - * Flush out locally pending IO statistics + * Flush out locally pending backend statistics * * If no stats have been recorded, this function returns false. - * - * If nowait is true, this function returns true if the lock could not be - * acquired. Otherwise, return false. */ bool -pgstat_io_flush_cb(bool nowait) +pgstat_per_backend_flush_cb(PgStat_EntryRef *entry_ref, bool nowait) { - LWLock *bktype_lock; + PgStatShared_Backend *shbackendioent; + PgStat_PendingIO *pendingent; PgStat_BktypeIO *bktype_shstats; + LWLock *bktype_lock; + PgStat_BktypeIO *bktype_global_shstats; - if (!have_iostats) + if (!pgstat_lock_entry(entry_ref, nowait)) return false; + /* global IO stats */ bktype_lock = &pgStatLocal.shmem->io.locks[MyBackendType]; - bktype_shstats = - &pgStatLocal.shmem->io.stats.stats[MyBackendType]; + bktype_global_shstats = &pgStatLocal.shmem->io.stats.stats[MyBackendType]; if (!nowait) LWLockAcquire(bktype_lock, LW_EXCLUSIVE); else if (!LWLockConditionalAcquire(bktype_lock, LW_EXCLUSIVE)) - return true; - - for (int io_object = 0; io_object < IOOBJECT_NUM_TYPES; io_object++) { - for (int io_context = 0; io_context < IOCONTEXT_NUM_TYPES; io_context++) - { - for (int io_op = 0; io_op < IOOP_NUM_TYPES; io_op++) - { - instr_time time; - - bktype_shstats->counts[io_object][io_context][io_op] += - PendingIOStats.counts[io_object][io_context][io_op]; - - time = PendingIOStats.pending_times[io_object][io_context][io_op]; - - bktype_shstats->times[io_object][io_context][io_op] += - INSTR_TIME_GET_MICROSEC(time); - } - } - } - - Assert(pgstat_bktype_io_stats_valid(bktype_shstats, MyBackendType)); - - LWLockRelease(bktype_lock); - - memset(&PendingIOStats, 0, sizeof(PendingIOStats)); - - have_iostats = false; - - return false; -} - -/* - * Flush out locally pending backend statistics - * - * If no stats have been recorded, this function returns false. - */ -bool -pgstat_per_backend_flush_cb(PgStat_EntryRef *entry_ref, bool nowait) -{ - PgStatShared_Backend *shbackendioent; - PgStat_PendingIO *pendingent; - PgStat_BktypeIO *bktype_shstats; - - if (!pgstat_lock_entry(entry_ref, nowait)) + pgstat_unlock_entry(entry_ref); return false; + } + /* per backend IO stats */ shbackendioent = (PgStatShared_Backend *) entry_ref->shared_stats; bktype_shstats = &shbackendioent->stats.stats; pendingent = (PgStat_PendingIO *) entry_ref->pending; @@ -279,6 +218,7 @@ pgstat_per_backend_flush_cb(PgStat_EntryRef *entry_ref, bool nowait) { instr_time time; + /* per backend IO stats */ bktype_shstats->counts[io_object][io_context][io_op] += pendingent->counts[io_object][io_context][io_op]; @@ -286,10 +226,19 @@ pgstat_per_backend_flush_cb(PgStat_EntryRef *entry_ref, bool nowait) bktype_shstats->times[io_object][io_context][io_op] += INSTR_TIME_GET_MICROSEC(time); + + /* global IO stats */ + bktype_global_shstats->counts[io_object][io_context][io_op] += + pendingent->counts[io_object][io_context][io_op]; + + bktype_global_shstats->times[io_object][io_context][io_op] += + INSTR_TIME_GET_MICROSEC(time); } } } + LWLockRelease(bktype_lock); + pgstat_unlock_entry(entry_ref); return true; -- 2.34.1 [text/x-diff] v5-0003-Don-t-include-other-backend-s-stats-in-the-snapsh.patch (1018B, ../../[email protected]/4-v5-0003-Don-t-include-other-backend-s-stats-in-the-snapsh.patch) download | inline diff: From 2d710a44af4d2758a0f33703893712693dfaf0af Mon Sep 17 00:00:00 2001 From: Bertrand Drouvot <[email protected]> Date: Thu, 7 Nov 2024 12:10:37 +0000 Subject: [PATCH v5 3/4] Don't include other backend's stats in the snapshot When stats_fetch_consistency is set to 'snapshot', don't include other backend's stats in the snapshot. There is no use case, so save memory usage. --- src/backend/utils/activity/pgstat.c | 4 ++++ 1 file changed, 4 insertions(+) 100.0% src/backend/utils/activity/ diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c index d051db5c10..4982dec8a9 100644 --- a/src/backend/utils/activity/pgstat.c +++ b/src/backend/utils/activity/pgstat.c @@ -1181,6 +1181,10 @@ pgstat_build_snapshot(void) !kind_info->accessed_across_databases) continue; + /* there is no need to include other backend's stats */ + if (kind == PGSTAT_KIND_PER_BACKEND && p->key.objid != MyProcNumber) + continue; + if (p->dropped) continue; -- 2.34.1 [text/x-diff] v5-0004-Add-pg_stat_get_backend_io.patch (15.1K, ../../[email protected]/5-v5-0004-Add-pg_stat_get_backend_io.patch) download | inline diff: From 2bf372f079e9cbecd82ce431622f7e683f659596 Mon Sep 17 00:00:00 2001 From: Bertrand Drouvot <[email protected]> Date: Thu, 7 Nov 2024 14:27:05 +0000 Subject: [PATCH v5 4/4] Add pg_stat_get_backend_io() Adding the pg_stat_get_backend_io() function to retrieve I/O statistics for a particular backend pid. Note that this function does not return any rows if stats_fetch_consistency is set to 'snapshot' and the pid of interest is not our own pid (there is no use case of retrieving other backend's stats with stats_fetch_consistency set to 'snapshot'). --- doc/src/sgml/monitoring.sgml | 17 ++++++++ src/backend/catalog/system_views.sql | 2 +- src/backend/utils/activity/pgstat_io.c | 18 ++++++++ src/backend/utils/adt/pgstatfuncs.c | 27 +++++++++++- src/include/catalog/pg_proc.dat | 14 +++---- src/include/pgstat.h | 1 + src/test/regress/expected/rules.out | 2 +- src/test/regress/expected/stats.out | 57 ++++++++++++++++++++++++++ src/test/regress/sql/stats.sql | 33 +++++++++++++++ 9 files changed, 160 insertions(+), 11 deletions(-) 10.4% doc/src/sgml/ 5.8% src/backend/utils/activity/ 8.6% src/backend/utils/adt/ 16.0% src/include/catalog/ 32.9% src/test/regress/expected/ 24.1% src/test/regress/sql/ diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml index fc6aded3da..a6672548b8 100644 --- a/doc/src/sgml/monitoring.sgml +++ b/doc/src/sgml/monitoring.sgml @@ -4814,6 +4814,23 @@ description | Waiting for a newly initialized WAL file to reach durable storage </para></entry> </row> + <row> + <entry role="func_table_entry"><para role="func_signature"> + <indexterm> + <primary>pg_stat_get_backend_io</primary> + </indexterm> + <function>pg_stat_get_backend_io</function> ( <type>integer</type> ) + <returnvalue>setof record</returnvalue> + </para> + <para> + Returns I/O statistics about the backend with the specified + process ID. The output fields are exactly the same as the ones in the + <link linkend="monitoring-pg-stat-io-view"> <structname>pg_stat_io</structname></link> + view. This function does not return any rows if <varname>stats_fetch_consistency</varname> + is set to <literal>snapshot</literal> and the process ID is not our own. + </para></entry> + </row> + <row> <entry role="func_table_entry"><para role="func_signature"> <indexterm> diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql index 09af4a40a8..6f17f505f1 100644 --- a/src/backend/catalog/system_views.sql +++ b/src/backend/catalog/system_views.sql @@ -1190,7 +1190,7 @@ SELECT b.fsyncs, b.fsync_time, b.stats_reset -FROM pg_stat_get_my_io() b; +FROM pg_stat_get_backend_io(NULL) b; CREATE VIEW pg_stat_wal AS SELECT diff --git a/src/backend/utils/activity/pgstat_io.c b/src/backend/utils/activity/pgstat_io.c index 1384f8103e..1063c6bec3 100644 --- a/src/backend/utils/activity/pgstat_io.c +++ b/src/backend/utils/activity/pgstat_io.c @@ -162,6 +162,24 @@ pgstat_fetch_my_stat_io(void) pgstat_fetch_entry(PGSTAT_KIND_PER_BACKEND, InvalidOid, MyProcNumber); } +/* + * Returns other backend's IO stats or NULL if pgstat_fetch_consistency is set + * to 'snapshot'. + */ +PgStat_Backend * +pgstat_fetch_proc_stat_io(ProcNumber procNumber) +{ + PgStat_Backend *backend_entry; + + if (pgstat_fetch_consistency == PGSTAT_FETCH_CONSISTENCY_SNAPSHOT) + return NULL; + + backend_entry = (PgStat_Backend *) pgstat_fetch_entry(PGSTAT_KIND_PER_BACKEND, + InvalidOid, procNumber); + + return backend_entry; +} + /* * Simpler wrapper of pgstat_per_backend_flush_cb(). */ diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c index fd16986156..e2041be237 100644 --- a/src/backend/utils/adt/pgstatfuncs.c +++ b/src/backend/utils/adt/pgstatfuncs.c @@ -1475,7 +1475,7 @@ pg_stat_get_io(PG_FUNCTION_ARGS) } Datum -pg_stat_get_my_io(PG_FUNCTION_ARGS) +pg_stat_get_backend_io(PG_FUNCTION_ARGS) { ReturnSetInfo *rsinfo; PgStat_Backend *backend_stats; @@ -1487,7 +1487,30 @@ pg_stat_get_my_io(PG_FUNCTION_ARGS) InitMaterializedSRF(fcinfo, 0); rsinfo = (ReturnSetInfo *) fcinfo->resultinfo; - backend_stats = pgstat_fetch_my_stat_io(); + if (PG_ARGISNULL(0) || PG_GETARG_INT32(0) == MyProcPid) + backend_stats = pgstat_fetch_my_stat_io(); + else + { + PGPROC *proc; + ProcNumber procNumber; + int pid = PG_GETARG_INT32(0); + + proc = BackendPidGetProc(pid); + + /* maybe an auxiliary process? */ + if (proc == NULL) + proc = AuxiliaryPidGetProc(pid); + + if (proc != NULL) + { + procNumber = GetNumberFromPGProc(proc); + backend_stats = pgstat_fetch_proc_stat_io(procNumber); + if (!backend_stats) + return (Datum) 0; + } + else + return (Datum) 0; + } bktype = backend_stats->bktype; bktype_desc = CStringGetTextDatum(GetBackendTypeDesc(bktype)); diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat index 89eb89efe4..fafee5a947 100644 --- a/src/include/catalog/pg_proc.dat +++ b/src/include/catalog/pg_proc.dat @@ -5903,14 +5903,14 @@ proargnames => '{backend_type,object,context,reads,read_time,writes,write_time,writebacks,writeback_time,extends,extend_time,op_bytes,hits,evictions,reuses,fsyncs,fsync_time,stats_reset}', prosrc => 'pg_stat_get_io' }, -{ oid => '8806', descr => 'statistics: my backend IO statistics', - proname => 'pg_stat_get_my_io', prorows => '5', proretset => 't', +{ oid => '8806', descr => 'statistics: per backend IO statistics', + proname => 'pg_stat_get_backend_io', prorows => '5', proretset => 't', provolatile => 'v', proparallel => 'r', prorettype => 'record', - proargtypes => '', - proallargtypes => '{text,text,text,int8,float8,int8,float8,int8,float8,int8,float8,int8,int8,int8,int8,int8,float8,timestamptz}', - proargmodes => '{o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o}', - proargnames => '{backend_type,object,context,reads,read_time,writes,write_time,writebacks,writeback_time,extends,extend_time,op_bytes,hits,evictions,reuses,fsyncs,fsync_time,stats_reset}', - prosrc => 'pg_stat_get_my_io' }, + proargtypes => 'int4', proisstrict => 'f', + proallargtypes => '{int4,text,text,text,int8,float8,int8,float8,int8,float8,int8,float8,int8,int8,int8,int8,int8,float8,timestamptz}', + proargmodes => '{i,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o}', + proargnames => '{backend_pid,backend_type,object,context,reads,read_time,writes,write_time,writebacks,writeback_time,extends,extend_time,op_bytes,hits,evictions,reuses,fsyncs,fsync_time,stats_reset}', + prosrc => 'pg_stat_get_backend_io' }, { oid => '1136', descr => 'statistics: information about WAL activity', proname => 'pg_stat_get_wal', proisstrict => 'f', provolatile => 's', diff --git a/src/include/pgstat.h b/src/include/pgstat.h index 13b9c71759..54c9f8b45c 100644 --- a/src/include/pgstat.h +++ b/src/include/pgstat.h @@ -576,6 +576,7 @@ extern void pgstat_count_io_op_time(IOObject io_object, IOContext io_context, extern PgStat_IO *pgstat_fetch_stat_io(void); extern PgStat_Backend *pgstat_fetch_my_stat_io(void); +extern PgStat_Backend *pgstat_fetch_proc_stat_io(ProcNumber procNumber); extern const char *pgstat_get_io_context_name(IOContext io_context); extern const char *pgstat_get_io_object_name(IOObject io_object); diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out index f92516b047..3cc0cfbcb5 100644 --- a/src/test/regress/expected/rules.out +++ b/src/test/regress/expected/rules.out @@ -1416,7 +1416,7 @@ pg_my_stat_io| SELECT backend_type, fsyncs, fsync_time, stats_reset - FROM pg_stat_get_my_io() b(backend_type, object, context, reads, read_time, writes, write_time, writebacks, writeback_time, extends, extend_time, op_bytes, hits, evictions, reuses, fsyncs, fsync_time, stats_reset); + FROM pg_stat_get_backend_io(NULL::integer) b(backend_type, object, context, reads, read_time, writes, write_time, writebacks, writeback_time, extends, extend_time, op_bytes, hits, evictions, reuses, fsyncs, fsync_time, stats_reset); pg_policies| SELECT n.nspname AS schemaname, c.relname AS tablename, pol.polname AS policyname, diff --git a/src/test/regress/expected/stats.out b/src/test/regress/expected/stats.out index 846e693483..846ffb59f2 100644 --- a/src/test/regress/expected/stats.out +++ b/src/test/regress/expected/stats.out @@ -1263,12 +1263,18 @@ SELECT sum(extends) AS io_sum_shared_before_extends FROM pg_stat_io WHERE context = 'normal' AND object = 'relation' \gset SELECT sum(extends) AS my_io_sum_shared_before_extends FROM pg_my_stat_io WHERE context = 'normal' AND object = 'relation' \gset +SELECT sum(extends) AS backend_io_sum_shared_before_extends + FROM pg_stat_get_backend_io(pg_backend_pid()) + WHERE context = 'normal' AND object = 'relation' \gset SELECT sum(writes) AS writes, sum(fsyncs) AS fsyncs FROM pg_stat_io WHERE object = 'relation' \gset io_sum_shared_before_ SELECT sum(writes) AS writes, sum(fsyncs) AS fsyncs FROM pg_my_stat_io WHERE object = 'relation' \gset my_io_sum_shared_before_ +SELECT sum(writes) AS writes, sum(fsyncs) AS fsyncs + FROM pg_stat_get_backend_io(pg_backend_pid()) + WHERE object = 'relation' \gset backend_io_sum_shared_before_ CREATE TABLE test_io_shared(a int); INSERT INTO test_io_shared SELECT i FROM generate_series(1,100)i; SELECT pg_stat_force_next_flush(); @@ -1293,6 +1299,15 @@ SELECT :my_io_sum_shared_after_extends > :my_io_sum_shared_before_extends; t (1 row) +SELECT sum(extends) AS backend_io_sum_shared_after_extends + FROM pg_stat_get_backend_io(pg_backend_pid()) + WHERE context = 'normal' AND object = 'relation' \gset +SELECT :backend_io_sum_shared_after_extends > :backend_io_sum_shared_before_extends; + ?column? +---------- + t +(1 row) + -- After a checkpoint, there should be some additional IOCONTEXT_NORMAL writes -- and fsyncs in the global stats (not for the backend). -- See comment above for rationale for two explicit CHECKPOINTs. @@ -1331,6 +1346,48 @@ SELECT current_setting('fsync') = 'off' t (1 row) +-- Check the stats_fetch_consistency behavior on per backend I/O stats +SELECT pid AS checkpointer_pid FROM pg_stat_activity + WHERE backend_type = 'checkpointer' \gset +BEGIN; +SET LOCAL stats_fetch_consistency = snapshot; +SELECT sum(extends) AS my_io_sum_shared_before_extends + FROM pg_my_stat_io WHERE context = 'normal' AND object = 'relation' \gset +SELECT sum(extends) AS backend_io_sum_shared_before_extends + FROM pg_stat_get_backend_io(pg_backend_pid()) + WHERE context = 'normal' AND object = 'relation' \gset +INSERT INTO test_io_shared SELECT i FROM generate_series(1,100)i; +SELECT pg_stat_force_next_flush(); + pg_stat_force_next_flush +-------------------------- + +(1 row) + +SELECT sum(extends) AS my_io_sum_shared_after_extends + FROM pg_my_stat_io WHERE context = 'normal' AND object = 'relation' \gset +SELECT :my_io_sum_shared_after_extends = :my_io_sum_shared_before_extends; + ?column? +---------- + t +(1 row) + +SELECT sum(extends) AS backend_io_sum_shared_after_extends + FROM pg_stat_get_backend_io(pg_backend_pid()) + WHERE context = 'normal' AND object = 'relation' \gset +SELECT :backend_io_sum_shared_after_extends = :backend_io_sum_shared_before_extends; + ?column? +---------- + t +(1 row) + +-- Don't return any rows if querying other backend's stats with snapshot set +SELECT count(1) = 0 FROM pg_stat_get_backend_io(:checkpointer_pid); + ?column? +---------- + t +(1 row) + +ROLLBACK; -- Change the tablespace so that the table is rewritten directly, then SELECT -- from it to cause it to be read back into shared buffers. SELECT sum(reads) AS io_sum_shared_before_reads diff --git a/src/test/regress/sql/stats.sql b/src/test/regress/sql/stats.sql index 9cb14b7182..e597c4d597 100644 --- a/src/test/regress/sql/stats.sql +++ b/src/test/regress/sql/stats.sql @@ -611,12 +611,18 @@ SELECT sum(extends) AS io_sum_shared_before_extends FROM pg_stat_io WHERE context = 'normal' AND object = 'relation' \gset SELECT sum(extends) AS my_io_sum_shared_before_extends FROM pg_my_stat_io WHERE context = 'normal' AND object = 'relation' \gset +SELECT sum(extends) AS backend_io_sum_shared_before_extends + FROM pg_stat_get_backend_io(pg_backend_pid()) + WHERE context = 'normal' AND object = 'relation' \gset SELECT sum(writes) AS writes, sum(fsyncs) AS fsyncs FROM pg_stat_io WHERE object = 'relation' \gset io_sum_shared_before_ SELECT sum(writes) AS writes, sum(fsyncs) AS fsyncs FROM pg_my_stat_io WHERE object = 'relation' \gset my_io_sum_shared_before_ +SELECT sum(writes) AS writes, sum(fsyncs) AS fsyncs + FROM pg_stat_get_backend_io(pg_backend_pid()) + WHERE object = 'relation' \gset backend_io_sum_shared_before_ CREATE TABLE test_io_shared(a int); INSERT INTO test_io_shared SELECT i FROM generate_series(1,100)i; SELECT pg_stat_force_next_flush(); @@ -626,6 +632,10 @@ SELECT :io_sum_shared_after_extends > :io_sum_shared_before_extends; SELECT sum(extends) AS my_io_sum_shared_after_extends FROM pg_my_stat_io WHERE context = 'normal' AND object = 'relation' \gset SELECT :my_io_sum_shared_after_extends > :my_io_sum_shared_before_extends; +SELECT sum(extends) AS backend_io_sum_shared_after_extends + FROM pg_stat_get_backend_io(pg_backend_pid()) + WHERE context = 'normal' AND object = 'relation' \gset +SELECT :backend_io_sum_shared_after_extends > :backend_io_sum_shared_before_extends; -- After a checkpoint, there should be some additional IOCONTEXT_NORMAL writes -- and fsyncs in the global stats (not for the backend). @@ -647,6 +657,29 @@ SELECT current_setting('fsync') = 'off' OR (:my_io_sum_shared_after_fsyncs = :my_io_sum_shared_before_fsyncs AND :my_io_sum_shared_after_fsyncs= 0); +-- Check the stats_fetch_consistency behavior on per backend I/O stats +SELECT pid AS checkpointer_pid FROM pg_stat_activity + WHERE backend_type = 'checkpointer' \gset +BEGIN; +SET LOCAL stats_fetch_consistency = snapshot; +SELECT sum(extends) AS my_io_sum_shared_before_extends + FROM pg_my_stat_io WHERE context = 'normal' AND object = 'relation' \gset +SELECT sum(extends) AS backend_io_sum_shared_before_extends + FROM pg_stat_get_backend_io(pg_backend_pid()) + WHERE context = 'normal' AND object = 'relation' \gset +INSERT INTO test_io_shared SELECT i FROM generate_series(1,100)i; +SELECT pg_stat_force_next_flush(); +SELECT sum(extends) AS my_io_sum_shared_after_extends + FROM pg_my_stat_io WHERE context = 'normal' AND object = 'relation' \gset +SELECT :my_io_sum_shared_after_extends = :my_io_sum_shared_before_extends; +SELECT sum(extends) AS backend_io_sum_shared_after_extends + FROM pg_stat_get_backend_io(pg_backend_pid()) + WHERE context = 'normal' AND object = 'relation' \gset +SELECT :backend_io_sum_shared_after_extends = :backend_io_sum_shared_before_extends; +-- Don't return any rows if querying other backend's stats with snapshot set +SELECT count(1) = 0 FROM pg_stat_get_backend_io(:checkpointer_pid); +ROLLBACK; + -- Change the tablespace so that the table is rewritten directly, then SELECT -- from it to cause it to be read back into shared buffers. SELECT sum(reads) AS io_sum_shared_before_reads -- 2.34.1 ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: per backend I/O statistics @ 2024-11-14 06:31 Michael Paquier <[email protected]> parent: Bertrand Drouvot <[email protected]> 0 siblings, 2 replies; 32+ messages in thread From: Michael Paquier @ 2024-11-14 06:31 UTC (permalink / raw) To: Bertrand Drouvot <[email protected]>; +Cc: Nazir Bilal Yavuz <[email protected]>; Alvaro Herrera <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected] On Fri, Nov 08, 2024 at 02:09:30PM +0000, Bertrand Drouvot wrote: > ==== 0001 (the largest one) > > Introduces a new statistics KIND. It is named PGSTAT_KIND_PER_BACKEND as it could > be used in the future to store other statistics (than the I/O ones) per backend. > The new KIND is a variable-numbered one and has an automatic cap on the > maximum number of entries (as its hash key contains the proc number). > > There is no need to serialize the per backend I/O stats to disk (no point to > see stats for backends that do not exist anymore after a re-start), so a > new "to_serialize" field is added in the PgStat_KindInfo struct. > > It adds a new pg_my_stat_io view to display "my" backend I/O statistics. > > It also adds a new function pg_stat_reset_single_backend_io_counters() to be > able to reset the I/O stats for a given backend pid. > > It contains doc updates and dedicated tests. > > A few remarks: > > 1. one assertion in pgstat_drop_entry_internal() is not necessary true anymore > with this new stat kind. So, adding an extra bool as parameter to take care of it. Why? I have to admit that the addition of this argument at this level specific to a new stats kind feels really weird and it looks like a layer violation, while this happens only when resetting the whole pgstats state because we have loaded a portion of the stats but the file loaded was in such a state that we don't have a consistent picture. > 2. a new struct "PgStat_Backend" is created and does contain the backend type. > The backend type is used for filtering purpose when the stats are displayed. Is that necessary? We can guess that from the PID with a join in pg_stat_activity. pg_stat_get_backend_io() from 0004 returns a set of tuples for a specific PID, as well.. + /* save the bktype */ + if (kind == PGSTAT_KIND_PER_BACKEND) + bktype = ((PgStatShared_Backend *) header)->stats.bktype; [...] + /* restore the bktype */ + if (kind == PGSTAT_KIND_PER_BACKEND) + ((PgStatShared_Backend *) header)->stats.bktype = bktype; Including the backend type results in these blips in shared_stat_reset_contents() which should not have anything related to stats kinds and should remain neutral, as well. pgstat_prep_per_backend_pending() is used only in pgstat_io.c, so I guess that it should be static in this file rather than declared in pgstat.h? +typedef struct PgStat_PendingIO Perhaps this part should use a separate structure named "BackendPendingIO"? The definition of the structure has to be in pgstat.h as this is the pending_size of the new stats kind. It looks like it would be cleaner to keep PgStat_PendingIO local to pgstat_io.c, and define PgStat_PendingIO based on PgStat_BackendPendingIO? + /* + * Do serialize or not this kind of stats. + */ + bool to_serialize:1; Not sure that "serialize" is the best term that applies here. For pgstats entries, serialization refers to the matter of writing their entries with a "serialized" name because they have an undefined number when stored locally after a reload. I'd suggest to split this concept into its own patch, rename the flag as "write_to_file" (or what you think is suited), and also apply the flag in the fixed-numbered loop done in pgstat_write_statsfile() before going through the dshash. + <row> + <entry role="func_table_entry"><para role="func_signature"> + <indexterm> + <primary>pg_stat_reset_single_backend_io_counters</primary> + </indexterm> + <function>pg_stat_reset_single_backend_io_counters</function> ( <type>int4</type> ) + <returnvalue>void</returnvalue> This should document that the input argument is a PID. Is pg_my_stat_io() the best name ever? I'd suggest to just merge 0004 with 0001. Structurally, it may be cleaner to keep all the callbacks and the backend-I/O specific logic into a separate file, perhaps pgstat_io_backend.c or pgstat_backend_io? > ==== 0002 > > Merge both IO stats flush callback. There is no need to keep both callbacks. > > Merging both allows to save O(N^3) while looping on IOOBJECT_NUM_TYPES, > IOCONTEXT_NUM_TYPES and IOCONTEXT_NUM_TYPES. Not sure to be a fan of that, TBH, still I get the performance argument of the matter. Each stats kind has its own flush path, and this assumes that we'll never going to diverge in terms of the information maintained for each backend and the existing pg_stat_io. Perhaps there could be a divergence at some point? > === Remarks > > R1. The key field is still "objid" even if it stores the proc number. I think > that's fine and there is no need to rename it as it's related comment states: Let's not change the object ID and the hash key. The approach you are using here with a variable-numbered stats kinds and a lookup with the procnumber is sensible here. > R2. pg_stat_get_io() and pg_stat_get_backend_io() could probably be merged ( > duplicated code). That's not mandatory to provide the new per backend I/O stats > feature. So let's keep it as future work to ease the review. Not sure about that. > R3. There is no use case of retrieving other backend's IO stats with > stats_fetch_consistency set to 'snapshot'. Avoiding this behavior allows us to > save memory (that could be non negligeable as pgstat_build_snapshot() would add > _all_ the other backend's stats in the snapshot). I choose to document it and to > not return any rows: I think that's better than returning some data (that would > not "ensure" the snapshot consistency) or an error. I'm pretty sure that there is a sensible argument about being able to get a snapshot of the I/O stat of another backend and consider that a feature. For example, keeping a look at the activity of the checkpointer while doing a scan at a certain point in time? With a GUC, we could have both behaviors and control which one we want. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: per backend I/O statistics @ 2024-11-14 13:30 Bertrand Drouvot <[email protected]> parent: Michael Paquier <[email protected]> 1 sibling, 1 reply; 32+ messages in thread From: Bertrand Drouvot @ 2024-11-14 13:30 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: Nazir Bilal Yavuz <[email protected]>; Alvaro Herrera <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected] Hi, On Thu, Nov 14, 2024 at 03:31:51PM +0900, Michael Paquier wrote: > On Fri, Nov 08, 2024 at 02:09:30PM +0000, Bertrand Drouvot wrote: > > 1. one assertion in pgstat_drop_entry_internal() is not necessary true anymore > > with this new stat kind. So, adding an extra bool as parameter to take care of it. > > Why? I have to admit that the addition of this argument at this level > specific to a new stats kind feels really weird and it looks like a > layer violation, while this happens only when resetting the whole > pgstats state because we have loaded a portion of the stats but the > file loaded was in such a state that we don't have a consistent > picture. Yes. I agree that looks weird and I don't like it that much. But the assertion is not true anymore. If you: - change the arguments to false in the pgstat_drop_entry_internal() call in pgstat_drop_all_entries() - start the engine - kill -9 postgres - restart the engine You'll see the assert failing due to the startup process. I don't think it is doing something wrong though, just populating its backend stats. Do you have another view on it? > > 2. a new struct "PgStat_Backend" is created and does contain the backend type. > > The backend type is used for filtering purpose when the stats are displayed. > > Is that necessary? We can guess that from the PID with a join in > pg_stat_activity. pg_stat_get_backend_io() from 0004 returns a set of > tuples for a specific PID, as well.. How would that work for someone doing "select * from pg_stat_get_backend_io(<pid>)"? Do you mean copy/paste part of the code from pg_stat_get_activity() into pg_stat_get_backend_io() to get the backend type? That sounds like an idea, I'll have a look at it. > + /* save the bktype */ > + if (kind == PGSTAT_KIND_PER_BACKEND) > + bktype = ((PgStatShared_Backend *) header)->stats.bktype; > [...] > + /* restore the bktype */ > + if (kind == PGSTAT_KIND_PER_BACKEND) > + ((PgStatShared_Backend *) header)->stats.bktype = bktype; > > Including the backend type results in these blips in > shared_stat_reset_contents() which should not have anything related > to stats kinds and should remain neutral, as well. Yeah, we can simply get rid of it if we remove the backend type in PgStat_Backend. > pgstat_prep_per_backend_pending() is used only in pgstat_io.c, so I > guess that it should be static in this file rather than declared in > pgstat.h? Good catch, thanks! > +typedef struct PgStat_PendingIO > > Perhaps this part should use a separate structure named > "BackendPendingIO"? The definition of the structure has to be in > pgstat.h as this is the pending_size of the new stats kind. It looks > like it would be cleaner to keep PgStat_PendingIO local to > pgstat_io.c, and define PgStat_PendingIO based on > PgStat_BackendPendingIO? I see what you meean, what about simply "PgStat_BackendPending" in pgstat.h? > + /* > + * Do serialize or not this kind of stats. > + */ > + bool to_serialize:1; > > Not sure that "serialize" is the best term that applies here. For > pgstats entries, serialization refers to the matter of writing their > entries with a "serialized" name because they have an undefined number > when stored locally after a reload. I'd suggest to split this concept > into its own patch, rename the flag as "write_to_file" (or what you > think is suited), and also apply the flag in the fixed-numbered loop > done in pgstat_write_statsfile() before going through the dshash. Makes sense to create its own patch, will have a look. > + <row> > + <entry role="func_table_entry"><para role="func_signature"> > + <indexterm> > + <primary>pg_stat_reset_single_backend_io_counters</primary> > + </indexterm> > + <function>pg_stat_reset_single_backend_io_counters</function> ( <type>int4</type> ) > + <returnvalue>void</returnvalue> > > This should document that the input argument is a PID. Yeap, will add. > > Is pg_my_stat_io() the best name ever? Not 100% sure, but there is already "pg_my_temp_schema" so I thought that pg_my_stat_io would not be that bad. Open to suggestions though. > I'd suggest to just merge 0004 with 0001. Sure. > Structurally, it may be cleaner to keep all the callbacks and the > backend-I/O specific logic into a separate file, perhaps > pgstat_io_backend.c or pgstat_backend_io? Yeah, I was wondering the same. What about pgstat_backend.c (that would contain only one "section" dedicated to IO currently)? > > ==== 0002 > > > > Merge both IO stats flush callback. There is no need to keep both callbacks. > > > > Merging both allows to save O(N^3) while looping on IOOBJECT_NUM_TYPES, > > IOCONTEXT_NUM_TYPES and IOCONTEXT_NUM_TYPES. > > Not sure to be a fan of that, TBH, still I get the performance > argument of the matter. Each stats kind has its own flush path, and > this assumes that we'll never going to diverge in terms of the > information maintained for each backend and the existing pg_stat_io. > Perhaps there could be a divergence at some point? Yeah perhaps, so in case of divergence let's split "again"? I mean for the moment I don't see any reason to keep both and we have pros related to efficiency during flush. > > === Remarks > > > > R1. The key field is still "objid" even if it stores the proc number. I think > > that's fine and there is no need to rename it as it's related comment states: > > Let's not change the object ID and the hash key. The approach you are > using here with a variable-numbered stats kinds and a lookup with the > procnumber is sensible here. Agree, thanks for confirming. > > R2. pg_stat_get_io() and pg_stat_get_backend_io() could probably be merged ( > > duplicated code). That's not mandatory to provide the new per backend I/O stats > > feature. So let's keep it as future work to ease the review. > > Not sure about that. I don't have a strong opinion about this one, so I'm ok to not merge those. > > R3. There is no use case of retrieving other backend's IO stats with > > stats_fetch_consistency set to 'snapshot'. Avoiding this behavior allows us to > > save memory (that could be non negligeable as pgstat_build_snapshot() would add > > _all_ the other backend's stats in the snapshot). I choose to document it and to > > not return any rows: I think that's better than returning some data (that would > > not "ensure" the snapshot consistency) or an error. > > I'm pretty sure that there is a sensible argument about being able to > get a snapshot of the I/O stat of another backend and consider that a > feature. For example, keeping a look at the activity of the > checkpointer while doing a scan at a certain point in time? hm, not sure how the stats_fetch_consistency set to 'snapshot' could help here: - the checkpointer could write stuff for reason completly unrelated to "your" backend - if the idea is to 1. record checkpointer stats, 2. do stuff in the backend and 3. check again the checkpointer stats then I don't think setting stats_fetch_consistency to snapshot is needed at all. In fact it's quite the opposite as 1. and 3. would report the same values with stats_fetch_consistency set to 'snapshot' (if done in the same transaction). Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: per backend I/O statistics @ 2024-11-19 01:47 Michael Paquier <[email protected]> parent: Bertrand Drouvot <[email protected]> 0 siblings, 1 reply; 32+ messages in thread From: Michael Paquier @ 2024-11-19 01:47 UTC (permalink / raw) To: Bertrand Drouvot <[email protected]>; +Cc: Nazir Bilal Yavuz <[email protected]>; Alvaro Herrera <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected] On Thu, Nov 14, 2024 at 01:30:11PM +0000, Bertrand Drouvot wrote: > - change the arguments to false in the pgstat_drop_entry_internal() call in > pgstat_drop_all_entries() > - start the engine > - kill -9 postgres > - restart the engine > > You'll see the assert failing due to the startup process. I don't think it is > doing something wrong though, just populating its backend stats. Do you have > another view on it? It feels sad that we have to plug in the internals at this level for this particular case. Perhaps there is something to do with more callbacks. Or perhaps there is just no point in tracking the stats of auxiliary processes because we already have this data in the existing pg_stat_io already? > How would that work for someone doing "select * from pg_stat_get_backend_io(<pid>)"? > > Do you mean copy/paste part of the code from pg_stat_get_activity() into > pg_stat_get_backend_io() to get the backend type? That sounds like an idea, > I'll have a look at it. I was under the impression that we should keep PgStat_Backend for the reset_timestamp part, but drop BackendType as it can be guessed from pg_stat_activity itself. For example a LATERAL to grab the current live stats of these backends. >> +typedef struct PgStat_PendingIO >> >> Perhaps this part should use a separate structure named >> "BackendPendingIO"? The definition of the structure has to be in >> pgstat.h as this is the pending_size of the new stats kind. It looks >> like it would be cleaner to keep PgStat_PendingIO local to >> pgstat_io.c, and define PgStat_PendingIO based on >> PgStat_BackendPendingIO? > > I see what you meean, what about simply "PgStat_BackendPending" in pgstat.h? That should be OK. >> Structurally, it may be cleaner to keep all the callbacks and the >> backend-I/O specific logic into a separate file, perhaps >> pgstat_io_backend.c or pgstat_backend_io? > > Yeah, I was wondering the same. What about pgstat_backend.c (that would contain > only one "section" dedicated to IO currently)? pgstat_backend.c looks good to me. Could there be other stats than just IO, actually? Perhaps not, just asking.. > - if the idea is to 1. record checkpointer stats, 2. do stuff in the backend and > 3. check again the checkpointer stats then I don't think setting stats_fetch_consistency > to snapshot is needed at all. In fact it's quite the opposite as 1. and 3. would > report the same values with stats_fetch_consistency set to 'snapshot' (if done > in the same transaction). Hmm. Not sure. It looks like you're right to treat this in a separate patch as it would exist for different reasons than the original proposal. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: per backend I/O statistics @ 2024-11-19 16:28 Bertrand Drouvot <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 1 reply; 32+ messages in thread From: Bertrand Drouvot @ 2024-11-19 16:28 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: Nazir Bilal Yavuz <[email protected]>; Alvaro Herrera <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected] Hi, On Tue, Nov 19, 2024 at 10:47:53AM +0900, Michael Paquier wrote: > On Thu, Nov 14, 2024 at 01:30:11PM +0000, Bertrand Drouvot wrote: > > - change the arguments to false in the pgstat_drop_entry_internal() call in > > pgstat_drop_all_entries() > > - start the engine > > - kill -9 postgres > > - restart the engine > > > > You'll see the assert failing due to the startup process. I don't think it is > > doing something wrong though, just populating its backend stats. Do you have > > another view on it? > > It feels sad that we have to plug in the internals at this level for > this particular case. Perhaps there is something to do with more > callbacks. Or perhaps there is just no point in tracking the stats of > auxiliary processes because we already have this data in the existing > pg_stat_io already? We can not discard the "per backend" stats collection for auxiliary processes because those are the source for pg_stat_io too (once the 2 flush callbacks are merged). I have another idea: after a bit more of investigation it turns out that only the startup process is generating the failed assertion. So, for the startup process only, what about? - don't call pgstat_create_backend_stat() in pgstat_beinit()... - but call it in StartupXLOG() instead (after the stats are discarded or restored). That way we could get rid of the changes linked to the assertion and still handle the startup process particular case. Thoughts? > > How would that work for someone doing "select * from pg_stat_get_backend_io(<pid>)"? > > > > Do you mean copy/paste part of the code from pg_stat_get_activity() into > > pg_stat_get_backend_io() to get the backend type? That sounds like an idea, > > I'll have a look at it. > > I was under the impression that we should keep PgStat_Backend for the > reset_timestamp part, but drop BackendType as it can be guessed from > pg_stat_activity itself. Removing BackendType sounds doable, I'll look at it. > >> Structurally, it may be cleaner to keep all the callbacks and the > >> backend-I/O specific logic into a separate file, perhaps > >> pgstat_io_backend.c or pgstat_backend_io? > > > > Yeah, I was wondering the same. What about pgstat_backend.c (that would contain > > only one "section" dedicated to IO currently)? > > pgstat_backend.c looks good to me. Could there be other stats than > just IO, actually? Perhaps not, just asking.. Yeah that's the reason why I suggested "pgstat_backend.c". I would not be surprised if we add more "per backend" stats (that are not I/O related) in the future as the main machinery would be there. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: per backend I/O statistics @ 2024-11-20 00:01 Michael Paquier <[email protected]> parent: Bertrand Drouvot <[email protected]> 0 siblings, 1 reply; 32+ messages in thread From: Michael Paquier @ 2024-11-20 00:01 UTC (permalink / raw) To: Bertrand Drouvot <[email protected]>; +Cc: Nazir Bilal Yavuz <[email protected]>; Alvaro Herrera <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected] On Tue, Nov 19, 2024 at 04:28:55PM +0000, Bertrand Drouvot wrote: > So, for the startup process only, what about? > > - don't call pgstat_create_backend_stat() in pgstat_beinit()... > - but call it in StartupXLOG() instead (after the stats are discarded or restored). > > That way we could get rid of the changes linked to the assertion and still handle > the startup process particular case. Thoughts? Hmm. That may prove to be a good idea in the long-term. The startup process is a specific path kicked in at a very early stage, so it is also a bit weird that we'd try to insert statistics while we are going to reset them anyway a bit later. That may also be relevant for custom statistics, actually, especially if some calls happen in some hook paths taken before the reset is done. This could happen for injection point stats when loading it with shared_preload_libraries, actually, if you load, attach or run a callback at this early stage. The existing sanity checks are a safety net that we should not change or adapt for specific stats kinds conditions, IMO. Perhaps this had better be done as a patch of its own, on top of the rest. > Yeah that's the reason why I suggested "pgstat_backend.c". I would not be > surprised if we add more "per backend" stats (that are not I/O related) in the > future as the main machinery would be there. Okay. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: per backend I/O statistics @ 2024-11-20 14:10 Bertrand Drouvot <[email protected]> parent: Michael Paquier <[email protected]> 1 sibling, 1 reply; 32+ messages in thread From: Bertrand Drouvot @ 2024-11-20 14:10 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: Nazir Bilal Yavuz <[email protected]>; Alvaro Herrera <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected] Hi, On Thu, Nov 14, 2024 at 03:31:51PM +0900, Michael Paquier wrote: > + /* > + * Do serialize or not this kind of stats. > + */ > + bool to_serialize:1; > > Not sure that "serialize" is the best term that applies here. For > pgstats entries, serialization refers to the matter of writing their > entries with a "serialized" name because they have an undefined number > when stored locally after a reload. I'd suggest to split this concept > into its own patch, rename the flag as "write_to_file" Yeah, also this could useful for custom statistics. So I created a dedicated thread and a patch proposal (see [1]). [1]: https://www.postgresql.org/message-id/Zz3skBqzBncSFIuY%40ip-10-97-1-34.eu-west-3.compute.internal Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: per backend I/O statistics @ 2024-11-20 14:20 Bertrand Drouvot <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 1 reply; 32+ messages in thread From: Bertrand Drouvot @ 2024-11-20 14:20 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: Nazir Bilal Yavuz <[email protected]>; Alvaro Herrera <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected] Hi, On Wed, Nov 20, 2024 at 09:01:26AM +0900, Michael Paquier wrote: > On Tue, Nov 19, 2024 at 04:28:55PM +0000, Bertrand Drouvot wrote: > > So, for the startup process only, what about? > > > > - don't call pgstat_create_backend_stat() in pgstat_beinit()... > > - but call it in StartupXLOG() instead (after the stats are discarded or restored). > > > > That way we could get rid of the changes linked to the assertion and still handle > > the startup process particular case. Thoughts? > > Hmm. That may prove to be a good idea in the long-term. The startup > process is a specific path kicked in at a very early stage, so it is > also a bit weird that we'd try to insert statistics while we are going > to reset them anyway a bit later. Exactly. > That may also be relevant for > custom statistics, actually, especially if some calls happen in some > hook paths taken before the reset is done. This could happen for > injection point stats when loading it with shared_preload_libraries, > actually, if you load, attach or run a callback at this early stage. Right. I did not had in mind to go that far here (for the per backend stats needs). My idea was "just" to move the new pgstat_create_backend_stat() (which is related to per backend stats only) call at the right place in StartupXLOG() for the startup process only. As that's the startup process that will reset or restore the stats I think that makes sense. It looks like that what you have in mind is much more generic, what about: - Focus on this thread first and then move the call as proposed above - Think about a more generic idea later on (on the per-backend I/O stats is in). Thoughts? Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: per backend I/O statistics @ 2024-11-20 22:18 Michael Paquier <[email protected]> parent: Bertrand Drouvot <[email protected]> 0 siblings, 1 reply; 32+ messages in thread From: Michael Paquier @ 2024-11-20 22:18 UTC (permalink / raw) To: Bertrand Drouvot <[email protected]>; +Cc: Nazir Bilal Yavuz <[email protected]>; Alvaro Herrera <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected] On Wed, Nov 20, 2024 at 02:20:18PM +0000, Bertrand Drouvot wrote: > Right. I did not had in mind to go that far here (for the per backend stats > needs). My idea was "just" to move the new pgstat_create_backend_stat() (which > is related to per backend stats only) call at the right place in StartupXLOG() > for the startup process only. As that's the startup process that will reset > or restore the stats I think that makes sense. > > It looks like that what you have in mind is much more generic, what about: > > - Focus on this thread first and then move the call as proposed above > - Think about a more generic idea later on (on the per-backend I/O stats is > in). Moving pgstat_create_backend_stat() may be OK in the long-term, at least that would document why we need to care about the startup process. Still, moving only the call feels incomplete, so how about adding a boolean field in PgStat_ShmemControl that defaults to false when the shmem area is initialized in StatsShmemInit(), then switched to true once we know that the stats have been restored or reset by the startup process. Something like that should work to control the dshash inserts, I guess? -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: per backend I/O statistics @ 2024-11-21 00:42 Michael Paquier <[email protected]> parent: Bertrand Drouvot <[email protected]> 0 siblings, 0 replies; 32+ messages in thread From: Michael Paquier @ 2024-11-21 00:42 UTC (permalink / raw) To: Bertrand Drouvot <[email protected]>; +Cc: Nazir Bilal Yavuz <[email protected]>; Alvaro Herrera <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected] On Wed, Nov 20, 2024 at 02:10:23PM +0000, Bertrand Drouvot wrote: > Yeah, also this could useful for custom statistics. So I created a dedicated > thread and a patch proposal (see [1]). > > [1]: https://www.postgresql.org/message-id/Zz3skBqzBncSFIuY%40ip-10-97-1-34.eu-west-3.compute.internal Thanks for putting that on its own thread. Will look at it. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: per backend I/O statistics @ 2024-11-21 17:23 Bertrand Drouvot <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 1 reply; 32+ messages in thread From: Bertrand Drouvot @ 2024-11-21 17:23 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: Nazir Bilal Yavuz <[email protected]>; Alvaro Herrera <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected] Hi, On Thu, Nov 21, 2024 at 07:18:24AM +0900, Michael Paquier wrote: > On Wed, Nov 20, 2024 at 02:20:18PM +0000, Bertrand Drouvot wrote: > > Right. I did not had in mind to go that far here (for the per backend stats > > needs). My idea was "just" to move the new pgstat_create_backend_stat() (which > > is related to per backend stats only) call at the right place in StartupXLOG() > > for the startup process only. As that's the startup process that will reset > > or restore the stats I think that makes sense. > > > > It looks like that what you have in mind is much more generic, what about: > > > > - Focus on this thread first and then move the call as proposed above > > - Think about a more generic idea later on (on the per-backend I/O stats is > > in). > > Moving pgstat_create_backend_stat() may be OK in the long-term, at > least that would document why we need to care about the startup > process. Yeap. > Still, moving only the call feels incomplete, so how about adding a > boolean field in PgStat_ShmemControl that defaults to false when the > shmem area is initialized in StatsShmemInit(), then switched to true > once we know that the stats have been restored or reset by the startup > process. Something like that should work to control the dshash > inserts, I guess? I did some tests (on a primary and on a standby) and currently there is no call to pgstat_get_entry_ref() at all before we reach the stats reset or restore in StartupXLOG(). The first ones appear after "we really open for business" (quoting process_pm_child_exit()). Now, with the per-backend I/O stats patch in place, there is 3 processes that could call pgstat_get_entry_ref() before we reach the stats reset or restore in StartupXLOG() (observed by setting a breakpoint on the startup process before stats are reset or restored): - checkpointer - background writer - startup process All calls are for the new backend stat kind. It also makes sense to see non "regular" backends as the "regulars" are not allowed to connect yet. Results: they could collect some stats for nothing since a reset or restore will happen after. Now, if we want to prevent new entries to be created before the stats are reset or restored: - then the end results will be the same (means starting the instance with reset or restored stats). The only difference would be that they would not collect stats for "nothing" during this small time window. - it looks like that would lead to some non negligible refactoring: I started the coding and observed the potential impact. The reason is that all the code (at least the one I looked at) does not expect to receive a NULL value from pgstat_get_entry_ref() (when asking for creation) and we would need to take care of all the cases (all the stats, all backend type) for safety reason. So, given that: - the end result would be the same - the code changes would be non negligible (unless we have a better idea than pgstat_get_entry_ref() returning a NULL value). I think that might be valid to try to implement it (to avoid those processes to collect the stats for nothing during this small time window) but I am not sure that's worth it (as the end result would be the same and the code change non negligible). Thoughts? Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: per backend I/O statistics @ 2024-11-22 01:36 Michael Paquier <[email protected]> parent: Bertrand Drouvot <[email protected]> 0 siblings, 1 reply; 32+ messages in thread From: Michael Paquier @ 2024-11-22 01:36 UTC (permalink / raw) To: Bertrand Drouvot <[email protected]>; +Cc: Nazir Bilal Yavuz <[email protected]>; Alvaro Herrera <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected] On Thu, Nov 21, 2024 at 05:23:42PM +0000, Bertrand Drouvot wrote: > So, given that: > > - the end result would be the same > - the code changes would be non negligible (unless we have a better idea than > pgstat_get_entry_ref() returning a NULL value). Hmm. created_entry only matters for pgstat_init_function_usage(). All the other callers of pgstat_prep_pending_entry() pass a NULL value. This makes me wonder if there is an argument for reworking a bit that. There's a fat comment about the reason why, still, that would make the get interface for variable-numbered stats a lot leaner.. Not sure what to think about that. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: per backend I/O statistics @ 2024-11-22 07:49 Bertrand Drouvot <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 1 reply; 32+ messages in thread From: Bertrand Drouvot @ 2024-11-22 07:49 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: Nazir Bilal Yavuz <[email protected]>; Alvaro Herrera <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected] Hi, On Fri, Nov 22, 2024 at 10:36:29AM +0900, Michael Paquier wrote: > On Thu, Nov 21, 2024 at 05:23:42PM +0000, Bertrand Drouvot wrote: > > So, given that: > > > > - the end result would be the same > > - the code changes would be non negligible (unless we have a better idea than > > pgstat_get_entry_ref() returning a NULL value). > > Hmm. created_entry only matters for pgstat_init_function_usage(). > All the other callers of pgstat_prep_pending_entry() pass a NULL > value. I meant to say all the calls that passe "create" as true in pgstat_get_entry_ref(). Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: per backend I/O statistics @ 2024-11-25 01:06 Michael Paquier <[email protected]> parent: Bertrand Drouvot <[email protected]> 0 siblings, 0 replies; 32+ messages in thread From: Michael Paquier @ 2024-11-25 01:06 UTC (permalink / raw) To: Bertrand Drouvot <[email protected]>; +Cc: Nazir Bilal Yavuz <[email protected]>; Alvaro Herrera <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected] On Fri, Nov 22, 2024 at 07:49:58AM +0000, Bertrand Drouvot wrote: > On Fri, Nov 22, 2024 at 10:36:29AM +0900, Michael Paquier wrote: >> Hmm. created_entry only matters for pgstat_init_function_usage(). >> All the other callers of pgstat_prep_pending_entry() pass a NULL >> value. > > I meant to say all the calls that passe "create" as true in pgstat_get_entry_ref(). Ah, OK, I think that I see your point here. I am wondering how much this would matter as well for custom stats, but we're not there yet without at least one release out and folks try new things with these APIs and variable-numbered kinds. Allowing pgstat_prep_pending_entry() to return NULL even if "create" is true may be a good thing, at the end, because that's the only way I can see based on the current APIs where we could say "Sorry, but the stats have not been loaded yet, so you cannot try to do anything related to the dshash". From my view having a kind of barrier would be cleaner in the long run, but it's true that it may not be mandatory, as well. pg_stat_io is currently OK to be called because the stats are loaded for auxiliary processes because it uses fixed-numbered stats in shmem. And it means we already have early calls that add stats getting overwritten once the stats are loaded from the on-disk file (Am I getting this part right?). Anyway, do we really require that for the sake of this thread? We know that there's only one of each auxiliary process at a time, and they keep a footprint in pg_stat_io already. So we could just limit outselves to live database backends, WAL senders and autovacuum workers, everything that's not auxiliary and spawned on request? -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 32+ messages in thread
end of thread, other threads:[~2024-11-25 01:06 UTC | newest] Thread overview: 32+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2018-11-12 21:11 [PATCH 1/1] Add high key "continuescan" optimization. Peter Geoghegan <[email protected]> 2024-05-14 23:26 [PATCH v19 2/8] Row pattern recognition patch (parse/analysis). Tatsuo Ishii <[email protected]> 2024-09-13 16:09 Re: per backend I/O statistics Bertrand Drouvot <[email protected]> 2024-09-17 11:52 ` Re: per backend I/O statistics Nazir Bilal Yavuz <[email protected]> 2024-09-17 13:07 ` Re: per backend I/O statistics Bertrand Drouvot <[email protected]> 2024-09-17 13:47 ` Re: per backend I/O statistics Nazir Bilal Yavuz <[email protected]> 2024-09-17 13:56 ` Re: per backend I/O statistics Bertrand Drouvot <[email protected]> 2024-09-20 04:26 ` Re: per backend I/O statistics Michael Paquier <[email protected]> 2024-10-07 09:54 ` Re: per backend I/O statistics Bertrand Drouvot <[email protected]> 2024-10-08 04:46 ` Re: per backend I/O statistics Michael Paquier <[email protected]> 2024-10-08 16:28 ` Re: per backend I/O statistics Bertrand Drouvot <[email protected]> 2024-10-31 05:09 ` Re: per backend I/O statistics Bertrand Drouvot <[email protected]> 2024-11-04 10:01 ` Re: per backend I/O statistics Bertrand Drouvot <[email protected]> 2024-11-05 17:37 ` Re: per backend I/O statistics Bertrand Drouvot <[email protected]> 2024-11-05 23:39 ` Re: per backend I/O statistics Michael Paquier <[email protected]> 2024-11-06 13:51 ` Re: per backend I/O statistics Bertrand Drouvot <[email protected]> 2024-11-07 00:50 ` Re: per backend I/O statistics Michael Paquier <[email protected]> 2024-11-07 16:32 ` Re: per backend I/O statistics Bertrand Drouvot <[email protected]> 2024-11-08 14:09 ` Re: per backend I/O statistics Bertrand Drouvot <[email protected]> 2024-11-14 06:31 ` Re: per backend I/O statistics Michael Paquier <[email protected]> 2024-11-14 13:30 ` Re: per backend I/O statistics Bertrand Drouvot <[email protected]> 2024-11-19 01:47 ` Re: per backend I/O statistics Michael Paquier <[email protected]> 2024-11-19 16:28 ` Re: per backend I/O statistics Bertrand Drouvot <[email protected]> 2024-11-20 00:01 ` Re: per backend I/O statistics Michael Paquier <[email protected]> 2024-11-20 14:20 ` Re: per backend I/O statistics Bertrand Drouvot <[email protected]> 2024-11-20 22:18 ` Re: per backend I/O statistics Michael Paquier <[email protected]> 2024-11-21 17:23 ` Re: per backend I/O statistics Bertrand Drouvot <[email protected]> 2024-11-22 01:36 ` Re: per backend I/O statistics Michael Paquier <[email protected]> 2024-11-22 07:49 ` Re: per backend I/O statistics Bertrand Drouvot <[email protected]> 2024-11-25 01:06 ` Re: per backend I/O statistics Michael Paquier <[email protected]> 2024-11-20 14:10 ` Re: per backend I/O statistics Bertrand Drouvot <[email protected]> 2024-11-21 00:42 ` Re: per backend I/O statistics 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