public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v28 08/11] Add aggregates support in IVM 13+ messages / 5 participants [nested] [flat]
* [PATCH v28 08/11] Add aggregates support in IVM @ 2023-05-31 11:46 Yugo Nagata <[email protected]> 0 siblings, 0 replies; 13+ messages in thread From: Yugo Nagata @ 2023-05-31 11:46 UTC (permalink / raw) count, sum, adn avg are supported. As a restriction, expressions specified in GROUP BY must appear in the target list because tuples to be updated in IMMV are identified by using this group key. However, in the case of aggregates without GROUP BY, there is only one tuple in the view, so keys are not uses to identify tuples. When creating a IMMV, in addition to __ivm_count column, some hidden columns for each aggregate are added to the target list. For example, names of these hidden columns are ivm_count_avg and ivm_sum_avg for the average function, and so on. When a base table is modified, the aggregated values and related hidden columns are also updated as well as __ivm_count__. The way of update depends the kind of aggregate function.=E3=80=80Specifically, sum and count are updated by simply adding or subtracting delta value calculated from delta tables. avg is updated by using values of sum and count stored in views as hidden columns and deltas calculated from delta tables. About aggregate functions except "count()" (sum and avg), NULLs in input values are ignored, and the result of aggegate should be NULL when no rows are selected. To support this specification, the numbers of non-NULL input values are counted and stored in hidden columns. In the case of count(), count(x) returns zero when no rows are selected, but count(*) doesn't ignore NULL input. --- src/backend/commands/createas.c | 264 +++++++++++++++++-- src/backend/commands/matview.c | 433 ++++++++++++++++++++++++++++++-- src/include/commands/createas.h | 1 + 3 files changed, 661 insertions(+), 37 deletions(-) diff --git a/src/backend/commands/createas.c b/src/backend/commands/createa= s.c index 076f35ee6b..c8aa558f2e 100644 --- a/src/backend/commands/createas.c +++ b/src/backend/commands/createas.c @@ -54,14 +54,19 @@ #include "parser/parsetree.h" #include "parser/parse_clause.h" #include "parser/parse_func.h" +#include "parser/parse_type.h" #include "rewrite/rewriteHandler.h" +#include "rewrite/rewriteManip.h" #include "storage/smgr.h" #include "tcop/tcopprot.h" #include "utils/builtins.h" #include "utils/lsyscache.h" +#include "utils/regproc.h" +#include "utils/fmgroids.h" #include "utils/rel.h" #include "utils/rls.h" #include "utils/snapmgr.h" +#include "utils/syscache.h" =20 typedef struct { @@ -75,6 +80,11 @@ typedef struct BulkInsertState bistate; /* bulk insert state */ } DR_intorel; =20 +typedef struct +{ + bool has_agg; +} check_ivm_restriction_context; + /* utility functions for CTAS definition creation */ static ObjectAddress create_ctas_internal(List *attrList, IntoClause *into= ); static ObjectAddress create_ctas_nodata(List *tlist, IntoClause *into); @@ -89,8 +99,9 @@ static void CreateIvmTriggersOnBaseTablesRecurse(Query *q= ry, Node *node, Oid mat Relids *relids, bool ex_lock); static void CreateIvmTrigger(Oid relOid, Oid viewOid, int16 type, int16 ti= ming, bool ex_lock); static void check_ivm_restriction(Node *node); -static bool check_ivm_restriction_walker(Node *node, void *context); +static bool check_ivm_restriction_walker(Node *node, check_ivm_restriction= _context *context); static Bitmapset *get_primary_key_attnos_from_query(Query *query, List **c= onstraintList); +static bool check_aggregate_supports_ivm(Oid aggfnoid); =20 /* * create_ctas_internal @@ -421,6 +432,7 @@ ExecCreateTableAs(ParseState *pstate, CreateTableAsStmt= *stmt, * rewriteQueryForIMMV -- rewrite view definition query for IMMV * * count(*) is added for counting distinct tuples in views. + * Also, additional hidden columns are added for aggregate values. */ Query * rewriteQueryForIMMV(Query *query, List *colNames) @@ -434,16 +446,49 @@ rewriteQueryForIMMV(Query *query, List *colNames) rewritten =3D copyObject(query); pstate->p_expr_kind =3D EXPR_KIND_SELECT_TARGET; =20 - /* - * Convert DISTINCT to GROUP BY and add count(*) for counting distinct - * tuples in views. - */ - if (rewritten->distinctClause) + /* group keys must be in targetlist */ + if (rewritten->groupClause) { - TargetEntry *tle; + ListCell *lc; + foreach(lc, rewritten->groupClause) + { + SortGroupClause *scl =3D (SortGroupClause *) lfirst(lc); + TargetEntry *tle =3D get_sortgroupclause_tle(scl, rewritten->targetList= ); =20 + if (tle->resjunk) + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("GROUP BY expression not appearing in select list is not sup= ported on incrementally maintainable materialized view"))); + } + } + /* Convert DISTINCT to GROUP BY. count(*) will be added afterward. */ + else if (!rewritten->hasAggs && rewritten->distinctClause) rewritten->groupClause =3D transformDistinctClause(NULL, &rewritten->tar= getList, rewritten->sortClause, false); =20 + /* Add additional columns for aggregate values */ + if (rewritten->hasAggs) + { + ListCell *lc; + List *aggs =3D NIL; + AttrNumber next_resno =3D list_length(rewritten->targetList) + 1; + + foreach(lc, rewritten->targetList) + { + TargetEntry *tle =3D (TargetEntry *) lfirst(lc); + char *resname =3D (colNames =3D=3D NIL || foreach_current_index(lc) >= =3D list_length(colNames) ? + tle->resname : strVal(list_nth(colNames, tle->resno - 1))); + + if (IsA(tle->expr, Aggref)) + makeIvmAggColumn(pstate, (Aggref *) tle->expr, resname, &next_resno, &= aggs); + } + rewritten->targetList =3D list_concat(rewritten->targetList, aggs); + } + + /* Add count(*) for counting distinct tuples in views */ + if (rewritten->distinctClause || rewritten->hasAggs) + { + TargetEntry *tle; + fn =3D makeFuncCall(SystemFuncName("count"), NIL, COERCE_EXPLICIT_CALL, = -1); fn->agg_star =3D true; =20 @@ -460,6 +505,91 @@ rewriteQueryForIMMV(Query *query, List *colNames) return rewritten; } =20 +/* + * makeIvmAggColumn -- make additional aggregate columns for IVM + * + * For an aggregate column specified by aggref, additional aggregate colum= ns + * are added, which are used to calculate the new aggregate value in IMMV. + * An additional aggregate columns has a name based on resname + * (ex. ivm_count_resname), and resno specified by next_resno. The created + * columns are returned to aggs, and the resno for the next column is also + * returned to next_resno. + * + * Currently, an additional count() is created for aggref other than count. + * In addition, sum() is created for avg aggregate column. + */ +void +makeIvmAggColumn(ParseState *pstate, Aggref *aggref, char *resname, AttrNu= mber *next_resno, List **aggs) +{ + TargetEntry *tle_count; + Node *node; + FuncCall *fn; + Const *dmy_arg =3D makeConst(INT4OID, + -1, + InvalidOid, + sizeof(int32), + Int32GetDatum(1), + false, + true); /* pass by value */ + const char *aggname =3D get_func_name(aggref->aggfnoid); + + /* + * For aggregate functions except count, add count() func with the same a= rg parameters. + * This count result is used for determining if the aggregate value shoul= d be NULL or not. + * Also, add sum() func for avg because we need to calculate an average v= alue as sum/count. + * + * XXX: If there are same expressions explicitly in the target list, we c= an use this instead + * of adding new duplicated one. + */ + if (strcmp(aggname, "count") !=3D 0) + { + fn =3D makeFuncCall(SystemFuncName("count"), NIL, COERCE_EXPLICIT_CALL, = -1); + + /* Make a Func with a dummy arg, and then override this by the original = agg's args. */ + node =3D ParseFuncOrColumn(pstate, fn->funcname, list_make1(dmy_arg), NU= LL, fn, false, -1); + ((Aggref *)node)->args =3D aggref->args; + + tle_count =3D makeTargetEntry((Expr *) node, + *next_resno, + pstrdup(makeObjectName("__ivm_count",resname, "_")), + false); + *aggs =3D lappend(*aggs, tle_count); + (*next_resno)++; + } + if (strcmp(aggname, "avg") =3D=3D 0) + { + List *dmy_args =3D NIL; + ListCell *lc; + foreach(lc, aggref->aggargtypes) + { + Oid typeid =3D lfirst_oid(lc); + Type type =3D typeidType(typeid); + + Const *con =3D makeConst(typeid, + -1, + typeTypeCollation(type), + typeLen(type), + (Datum) 0, + true, + typeByVal(type)); + dmy_args =3D lappend(dmy_args, con); + ReleaseSysCache(type); + } + fn =3D makeFuncCall(SystemFuncName("sum"), NIL, COERCE_EXPLICIT_CALL, -1= ); + + /* Make a Func with dummy args, and then override this by the original a= gg's args. */ + node =3D ParseFuncOrColumn(pstate, fn->funcname, dmy_args, NULL, fn, fal= se, -1); + ((Aggref *)node)->args =3D aggref->args; + + tle_count =3D makeTargetEntry((Expr *) node, + *next_resno, + pstrdup(makeObjectName("__ivm_sum",resname, "_")), + false); + *aggs =3D lappend(*aggs, tle_count); + (*next_resno)++; + } +} + /* * GetIntoRelEFlags --- compute executor flags needed for CREATE TABLE AS * @@ -943,11 +1073,13 @@ CreateIvmTrigger(Oid relOid, Oid viewOid, int16 type= , int16 timing, bool ex_lock static void check_ivm_restriction(Node *node) { - check_ivm_restriction_walker(node, NULL); + check_ivm_restriction_context context =3D {false}; + + check_ivm_restriction_walker(node, &context); } =20 static bool -check_ivm_restriction_walker(Node *node, void *context) +check_ivm_restriction_walker(Node *node, check_ivm_restriction_context *co= ntext) { if (node =3D=3D NULL) return false; @@ -976,6 +1108,10 @@ check_ivm_restriction_walker(Node *node, void *contex= t) ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), errmsg("CTE is not supported on incrementally maintainable materia= lized view"))); + if (qry->groupClause !=3D NIL && !qry->hasAggs) + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("GROUP BY clause without aggregate is not supported on incr= ementally maintainable materialized view"))); if (qry->havingQual !=3D NULL) ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), @@ -1028,6 +1164,8 @@ check_ivm_restriction_walker(Node *node, void *contex= t) } } =20 + context->has_agg |=3D qry->hasAggs; + /* restrictions for rtable */ foreach(lc, qry->rtable) { @@ -1076,7 +1214,7 @@ check_ivm_restriction_walker(Node *node, void *contex= t) =20 } =20 - query_tree_walker(qry, check_ivm_restriction_walker, NULL, QTW_IGNORE_= RANGE_TABLE); + query_tree_walker(qry, check_ivm_restriction_walker, (void *) context,= QTW_IGNORE_RANGE_TABLE); =20 break; } @@ -1087,8 +1225,12 @@ check_ivm_restriction_walker(Node *node, void *conte= xt) ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), errmsg("column name %s is not supported on incrementally maintain= able materialized view", tle->resname))); + if (context->has_agg && !IsA(tle->expr, Aggref) && contain_aggs_of_lev= el((Node *) tle->expr, 0)) + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("expression containing an aggregate in it is not supported = on incrementally maintainable materialized view"))); =20 - expression_tree_walker(node, check_ivm_restriction_walker, NULL); + expression_tree_walker(node, check_ivm_restriction_walker, (void *) co= ntext); break; } case T_JoinExpr: @@ -1100,14 +1242,36 @@ check_ivm_restriction_walker(Node *node, void *cont= ext) (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), errmsg("OUTER JOIN is not supported on incrementally maintainable= materialized view"))); =20 - expression_tree_walker(node, check_ivm_restriction_walker, NULL); + expression_tree_walker(node, check_ivm_restriction_walker, (void *) co= ntext); + break; } - break; case T_Aggref: - ereport(ERROR, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("aggregate function is not supported on incrementally maintai= nable materialized view"))); - break; + { + /* Check if this supports IVM */ + Aggref *aggref =3D (Aggref *) node; + const char *aggname =3D format_procedure(aggref->aggfnoid); + + if (aggref->aggfilter !=3D NULL) + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("aggregate function with FILTER clause is not supported on = incrementally maintainable materialized view"))); + + if (aggref->aggdistinct !=3D NULL) + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("aggregate function with DISTINCT arguments is not supporte= d on incrementally maintainable materialized view"))); + + if (aggref->aggorder !=3D NULL) + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("aggregate function with ORDER clause is not supported on i= ncrementally maintainable materialized view"))); + + if (!check_aggregate_supports_ivm(aggref->aggfnoid)) + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("aggregate function %s is not supported on incrementally ma= intainable materialized view", aggname))); + break; + } default: expression_tree_walker(node, check_ivm_restriction_walker, (void *) con= text); break; @@ -1115,6 +1279,46 @@ check_ivm_restriction_walker(Node *node, void *conte= xt) return false; } =20 +/* + * check_aggregate_supports_ivm + * + * Check if the given aggregate function is supporting IVM + */ +static bool +check_aggregate_supports_ivm(Oid aggfnoid) +{ + switch (aggfnoid) + { + /* count */ + case F_COUNT_ANY: + case F_COUNT_: + + /* sum */ + case F_SUM_INT8: + case F_SUM_INT4: + case F_SUM_INT2: + case F_SUM_FLOAT4: + case F_SUM_FLOAT8: + case F_SUM_MONEY: + case F_SUM_INTERVAL: + case F_SUM_NUMERIC: + + /* avg */ + case F_AVG_INT8: + case F_AVG_INT4: + case F_AVG_INT2: + case F_AVG_NUMERIC: + case F_AVG_FLOAT4: + case F_AVG_FLOAT8: + case F_AVG_INTERVAL: + + return true; + + default: + return false; + } +} + /* * CreateIndexOnIMMV * @@ -1172,7 +1376,29 @@ CreateIndexOnIMMV(Query *query, Relation matviewRel) index->concurrent =3D false; index->if_not_exists =3D false; =20 - if (query->distinctClause) + if (query->groupClause) + { + /* create unique constraint on GROUP BY expression columns */ + foreach(lc, query->groupClause) + { + SortGroupClause *scl =3D (SortGroupClause *) lfirst(lc); + TargetEntry *tle =3D get_sortgroupclause_tle(scl, query->targetList); + Form_pg_attribute attr =3D TupleDescAttr(matviewRel->rd_att, tle->resno= - 1); + IndexElem *iparam; + + iparam =3D makeNode(IndexElem); + iparam->name =3D pstrdup(NameStr(attr->attname)); + iparam->expr =3D NULL; + iparam->indexcolname =3D NULL; + iparam->collation =3D NIL; + iparam->opclass =3D NIL; + iparam->opclassopts =3D NIL; + iparam->ordering =3D SORTBY_DEFAULT; + iparam->nulls_ordering =3D SORTBY_NULLS_DEFAULT; + index->indexParams =3D lappend(index->indexParams, iparam); + } + } + else if (query->distinctClause) { /* create unique constraint on all columns */ foreach(lc, query->targetList) @@ -1230,7 +1456,7 @@ CreateIndexOnIMMV(Query *query, Relation matviewRel) (errmsg("could not create an index on materialized view \"%s\" automa= tically", RelationGetRelationName(matviewRel)), errdetail("This target list does not have all the primary key column= s, " - "or this view does not contain DISTINCT clause."), + "or this view does not contain GROUP BY or DISTINCT clause."), errhint("Create an index on the materialized view for efficient incr= emental maintenance."))); return; } diff --git a/src/backend/commands/matview.c b/src/backend/commands/matview.c index 6d8382180a..aa6bf2694a 100644 --- a/src/backend/commands/matview.c +++ b/src/backend/commands/matview.c @@ -30,6 +30,7 @@ #include "catalog/pg_opclass.h" #include "catalog/pg_operator.h" #include "commands/cluster.h" +#include "commands/defrem.h" #include "commands/matview.h" #include "commands/tablecmds.h" #include "commands/tablespace.h" @@ -39,6 +40,7 @@ #include "executor/tstoreReceiver.h" #include "miscadmin.h" #include "nodes/makefuncs.h" +#include "optimizer/optimizer.h" #include "parser/analyze.h" #include "parser/parse_clause.h" #include "parser/parse_func.h" @@ -111,6 +113,13 @@ static HTAB *mv_trigger_info =3D NULL; =20 static bool in_delta_calculation =3D false; =20 +/* kind of IVM operation for the view */ +typedef enum +{ + IVM_ADD, + IVM_SUB +} IvmOp; + /* ENR name for materialized view delta */ #define NEW_DELTA_ENRNAME "new_delta" #define OLD_DELTA_ENRNAME "old_delta" @@ -142,7 +151,7 @@ static RangeTblEntry *get_prestate_rte(RangeTblEntry *r= te, MV_TriggerTable *tabl QueryEnvironment *queryEnv, Oid matviewid); static RangeTblEntry *replace_rte_with_delta(RangeTblEntry *rte, MV_Trigge= rTable *table, bool is_new, QueryEnvironment *queryEnv); -static Query *rewrite_query_for_counting(Query *query, ParseState *pstate); +static Query *rewrite_query_for_counting_and_aggregates(Query *query, Pars= eState *pstate); =20 static void calc_delta(MV_TriggerTable *table, int rte_index, Query *query, DestReceiver *dest_old, DestReceiver *dest_new, @@ -153,14 +162,27 @@ static Query *rewrite_query_for_postupdate_state(Quer= y *query, MV_TriggerTable * static void apply_delta(Oid matviewOid, Tuplestorestate *old_tuplestores, = Tuplestorestate *new_tuplestores, TupleDesc tupdesc_old, TupleDesc tupdesc_new, Query *query, bool use_count, char *count_colname); +static void append_set_clause_for_count(const char *resname, StringInfo bu= f_old, + StringInfo buf_new,StringInfo aggs_list); +static void append_set_clause_for_sum(const char *resname, StringInfo buf_= old, + StringInfo buf_new, StringInfo aggs_list); +static void append_set_clause_for_avg(const char *resname, StringInfo buf_= old, + StringInfo buf_new, StringInfo aggs_list, + const char *aggtype); +static char *get_operation_string(IvmOp op, const char *col, const char *a= rg1, const char *arg2, + const char* count_col, const char *castType); +static char *get_null_condition_string(IvmOp op, const char *arg1, const c= har *arg2, + const char* count_col); static void apply_old_delta(const char *matviewname, const char *deltaname= _old, List *keys); static void apply_old_delta_with_count(const char *matviewname, const char= *deltaname_old, - List *keys, const char *count_colname); + List *keys, StringInfo aggs_list, StringInfo aggs_set, + const char *count_colname); static void apply_new_delta(const char *matviewname, const char *deltaname= _new, StringInfo target_list); static void apply_new_delta_with_count(const char *matviewname, const char= * deltaname_new, - List *keys, StringInfo target_list, const char* count_colname); + List *keys, StringInfo target_list, StringInfo aggs_set, + const char* count_colname); static char *get_matching_condition_string(List *keys); static void generate_equal(StringInfo querybuf, Oid opttype, const char *leftop, const char *rightop); @@ -1432,11 +1454,44 @@ IVM_immediate_maintenance(PG_FUNCTION_ARGS) * When a base table is truncated, the view content will be empty if the * view definition query does not contain an aggregate without a GROUP cl= ause. * Therefore, such views can be truncated. + * + * Aggregate views without a GROUP clause always have one row. Therefore, + * if a base table is truncated, the view will not be empty and will cont= ain + * a row with NULL value (or 0 for count()). So, in this case, we refresh= the + * view instead of truncating it. */ if (TRIGGER_FIRED_BY_TRUNCATE(trigdata->tg_event)) { - ExecuteTruncateGuts(list_make1(matviewRel), list_make1_oid(matviewOid), - NIL, DROP_RESTRICT, false, false); + if (!(query->hasAggs && query->groupClause =3D=3D NIL)) + ExecuteTruncateGuts(list_make1(matviewRel), list_make1_oid(matviewOid), + NIL, DROP_RESTRICT, false, false); + else + { + Oid OIDNewHeap; + DestReceiver *dest; + uint64 processed =3D 0; + Query *dataQuery =3D rewriteQueryForIMMV(query, NIL); + char relpersistence =3D matviewRel->rd_rel->relpersistence; + + /* + * Create the transient table that will receive the regenerated data. L= ock + * it against access by any other process until commit (by which time it + * will be gone). + */ + OIDNewHeap =3D make_new_heap(matviewOid, matviewRel->rd_rel->reltablesp= ace, + matviewRel->rd_rel->relam, + relpersistence, ExclusiveLock); + LockRelationOid(OIDNewHeap, AccessExclusiveLock); + dest =3D CreateTransientRelDestReceiver(OIDNewHeap); + + /* Generate the data */ + processed =3D refresh_matview_datafill(dest, dataQuery, NULL, NULL, ""); + refresh_by_heap_swap(matviewOid, OIDNewHeap, relpersistence); + + /* Inform cumulative stats system about our activity */ + pgstat_count_truncate(matviewRel); + pgstat_count_heap_insert(matviewRel, processed); + } =20 /* Clean up hash entry and delete tuplestores */ clean_up_IVM_hash_entry(entry, false); @@ -1476,8 +1531,8 @@ IVM_immediate_maintenance(PG_FUNCTION_ARGS) /* Set all tables in the query to pre-update state */ rewritten =3D rewrite_query_for_preupdate_state(rewritten, entry->tables, pstate, matviewOid); - /* Rewrite for counting duplicated tuples */ - rewritten =3D rewrite_query_for_counting(rewritten, pstate); + /* Rewrite for counting duplicated tuples and aggregates functions*/ + rewritten =3D rewrite_query_for_counting_and_aggregates(rewritten, pstate= ); =20 /* Create tuplestores to store view deltas */ if (entry->has_old) @@ -1528,7 +1583,7 @@ IVM_immediate_maintenance(PG_FUNCTION_ARGS) =20 count_colname =3D pstrdup("__ivm_count__"); =20 - if (query->distinctClause) + if (query->hasAggs || query->distinctClause) use_count =3D true; =20 /* calculate delta tables */ @@ -1924,17 +1979,34 @@ replace_rte_with_delta(RangeTblEntry *rte, MV_Trigg= erTable *table, bool is_new, } =20 /* - * rewrite_query_for_counting + * rewrite_query_for_counting_and_aggregates * - * Rewrite query for counting duplicated tuples. + * Rewrite query for counting duplicated tuples and aggregate functions. */ static Query * -rewrite_query_for_counting(Query *query, ParseState *pstate) +rewrite_query_for_counting_and_aggregates(Query *query, ParseState *pstate) { TargetEntry *tle_count; FuncCall *fn; Node *node; =20 + /* For aggregate views */ + if (query->hasAggs) + { + ListCell *lc; + List *aggs =3D NIL; + AttrNumber next_resno =3D list_length(query->targetList) + 1; + + foreach(lc, query->targetList) + { + TargetEntry *tle =3D (TargetEntry *) lfirst(lc); + + if (IsA(tle->expr, Aggref)) + makeIvmAggColumn(pstate, (Aggref *)tle->expr, tle->resname, &next_resn= o, &aggs); + } + query->targetList =3D list_concat(query->targetList, aggs); + } + /* Add count(*) for counting distinct tuples in views */ fn =3D makeFuncCall(SystemFuncName("count"), NIL, COERCE_EXPLICIT_CALL, -= 1); fn->agg_star =3D true; @@ -2007,6 +2079,8 @@ rewrite_query_for_postupdate_state(Query *query, MV_T= riggerTable *table, int rte return query; } =20 +#define IVM_colname(type, col) makeObjectName("__ivm_" type, col, "_") + /* * apply_delta * @@ -2020,6 +2094,9 @@ apply_delta(Oid matviewOid, Tuplestorestate *old_tupl= estores, Tuplestorestate *n { StringInfoData querybuf; StringInfoData target_list_buf; + StringInfo aggs_list_buf =3D NULL; + StringInfo aggs_set_old =3D NULL; + StringInfo aggs_set_new =3D NULL; Relation matviewRel; char *matviewname; ListCell *lc; @@ -2042,6 +2119,15 @@ apply_delta(Oid matviewOid, Tuplestorestate *old_tup= lestores, Tuplestorestate *n initStringInfo(&querybuf); initStringInfo(&target_list_buf); =20 + if (query->hasAggs) + { + if (old_tuplestores && tuplestore_tuple_count(old_tuplestores) > 0) + aggs_set_old =3D makeStringInfo(); + if (new_tuplestores && tuplestore_tuple_count(new_tuplestores) > 0) + aggs_set_new =3D makeStringInfo(); + aggs_list_buf =3D makeStringInfo(); + } + /* build string of target list */ for (i =3D 0; i < matviewRel->rd_att->natts; i++) { @@ -2058,13 +2144,61 @@ apply_delta(Oid matviewOid, Tuplestorestate *old_tu= plestores, Tuplestorestate *n { TargetEntry *tle =3D (TargetEntry *) lfirst(lc); Form_pg_attribute attr =3D TupleDescAttr(matviewRel->rd_att, i); + char *resname =3D NameStr(attr->attname); =20 i++; =20 if (tle->resjunk) continue; =20 - keys =3D lappend(keys, attr); + /* + * For views without aggregates, all attributes are used as keys to iden= tify a + * tuple in a view. + */ + if (!query->hasAggs) + keys =3D lappend(keys, attr); + + /* For views with aggregates, we need to build SET clause for updating a= ggregate + * values. */ + if (query->hasAggs && IsA(tle->expr, Aggref)) + { + Aggref *aggref =3D (Aggref *) tle->expr; + const char *aggname =3D get_func_name(aggref->aggfnoid); + + /* + * We can use function names here because it is already checked if these + * can be used in IMMV by its OID at the definition time. + */ + + /* count */ + if (!strcmp(aggname, "count")) + append_set_clause_for_count(resname, aggs_set_old, aggs_set_new, aggs_= list_buf); + + /* sum */ + else if (!strcmp(aggname, "sum")) + append_set_clause_for_sum(resname, aggs_set_old, aggs_set_new, aggs_li= st_buf); + + /* avg */ + else if (!strcmp(aggname, "avg")) + append_set_clause_for_avg(resname, aggs_set_old, aggs_set_new, aggs_li= st_buf, + format_type_be(aggref->aggtype)); + + else + elog(ERROR, "unsupported aggregate function: %s", aggname); + } + } + + /* If we have GROUP BY clause, we use its entries as keys. */ + if (query->hasAggs && query->groupClause) + { + foreach (lc, query->groupClause) + { + SortGroupClause *sgcl =3D (SortGroupClause *) lfirst(lc); + TargetEntry *tle =3D get_sortgroupclause_tle(sgcl, query->targetList); + Form_pg_attribute attr =3D TupleDescAttr(matviewRel->rd_att, tle->resno= - 1); + + keys =3D lappend(keys, attr); + } } =20 /* Start maintaining the materialized view. */ @@ -2095,7 +2229,8 @@ apply_delta(Oid matviewOid, Tuplestorestate *old_tupl= estores, Tuplestorestate *n if (use_count) /* apply old delta and get rows to be recalculated */ apply_old_delta_with_count(matviewname, OLD_DELTA_ENRNAME, - keys, count_colname); + keys, aggs_list_buf, aggs_set_old, + count_colname); else apply_old_delta(matviewname, OLD_DELTA_ENRNAME, keys); =20 @@ -2121,7 +2256,7 @@ apply_delta(Oid matviewOid, Tuplestorestate *old_tupl= estores, Tuplestorestate *n /* apply new delta */ if (use_count) apply_new_delta_with_count(matviewname, NEW_DELTA_ENRNAME, - keys, &target_list_buf, count_colname); + keys, aggs_set_new, &target_list_buf, count_colname); else apply_new_delta(matviewname, NEW_DELTA_ENRNAME, &target_list_buf); } @@ -2136,6 +2271,250 @@ apply_delta(Oid matviewOid, Tuplestorestate *old_tu= plestores, Tuplestorestate *n elog(ERROR, "SPI_finish failed"); } =20 +/* + * append_set_clause_for_count + * + * Append SET clause string for count aggregation to given buffers. + * Also, append resnames required for calculating the aggregate value. + */ +static void +append_set_clause_for_count(const char *resname, StringInfo buf_old, + StringInfo buf_new,StringInfo aggs_list) +{ + /* For tuple deletion */ + if (buf_old) + { + /* resname =3D mv.resname - t.resname */ + appendStringInfo(buf_old, + ", %s =3D %s", + quote_qualified_identifier(NULL, resname), + get_operation_string(IVM_SUB, resname, "mv", "t", NULL, NULL)); + } + /* For tuple insertion */ + if (buf_new) + { + /* resname =3D mv.resname + diff.resname */ + appendStringInfo(buf_new, + ", %s =3D %s", + quote_qualified_identifier(NULL, resname), + get_operation_string(IVM_ADD, resname, "mv", "diff", NULL, NULL)); + } + + appendStringInfo(aggs_list, ", %s", + quote_qualified_identifier("diff", resname) + ); +} + +/* + * append_set_clause_for_sum + * + * Append SET clause string for sum aggregation to given buffers. + * Also, append resnames required for calculating the aggregate value. + */ +static void +append_set_clause_for_sum(const char *resname, StringInfo buf_old, + StringInfo buf_new, StringInfo aggs_list) +{ + char *count_col =3D IVM_colname("count", resname); + + /* For tuple deletion */ + if (buf_old) + { + /* sum =3D mv.sum - t.sum */ + appendStringInfo(buf_old, + ", %s =3D %s", + quote_qualified_identifier(NULL, resname), + get_operation_string(IVM_SUB, resname, "mv", "t", count_col, NULL) + ); + /* count =3D mv.count - t.count */ + appendStringInfo(buf_old, + ", %s =3D %s", + quote_qualified_identifier(NULL, count_col), + get_operation_string(IVM_SUB, count_col, "mv", "t", NULL, NULL) + ); + } + /* For tuple insertion */ + if (buf_new) + { + /* sum =3D mv.sum + diff.sum */ + appendStringInfo(buf_new, + ", %s =3D %s", + quote_qualified_identifier(NULL, resname), + get_operation_string(IVM_ADD, resname, "mv", "diff", count_col, NULL) + ); + /* count =3D mv.count + diff.count */ + appendStringInfo(buf_new, + ", %s =3D %s", + quote_qualified_identifier(NULL, count_col), + get_operation_string(IVM_ADD, count_col, "mv", "diff", NULL, NULL) + ); + } + + appendStringInfo(aggs_list, ", %s, %s", + quote_qualified_identifier("diff", resname), + quote_qualified_identifier("diff", IVM_colname("count", resname)) + ); +} + +/* + * append_set_clause_for_avg + * + * Append SET clause string for avg aggregation to given buffers. + * Also, append resnames required for calculating the aggregate value. + */ +static void +append_set_clause_for_avg(const char *resname, StringInfo buf_old, + StringInfo buf_new, StringInfo aggs_list, + const char *aggtype) +{ + char *sum_col =3D IVM_colname("sum", resname); + char *count_col =3D IVM_colname("count", resname); + + /* For tuple deletion */ + if (buf_old) + { + /* avg =3D (mv.sum - t.sum)::aggtype / (mv.count - t.count) */ + appendStringInfo(buf_old, + ", %s =3D %s OPERATOR(pg_catalog./) %s", + quote_qualified_identifier(NULL, resname), + get_operation_string(IVM_SUB, sum_col, "mv", "t", count_col, aggtype), + get_operation_string(IVM_SUB, count_col, "mv", "t", NULL, NULL) + ); + /* sum =3D mv.sum - t.sum */ + appendStringInfo(buf_old, + ", %s =3D %s", + quote_qualified_identifier(NULL, sum_col), + get_operation_string(IVM_SUB, sum_col, "mv", "t", count_col, NULL) + ); + /* count =3D mv.count - t.count */ + appendStringInfo(buf_old, + ", %s =3D %s", + quote_qualified_identifier(NULL, count_col), + get_operation_string(IVM_SUB, count_col, "mv", "t", NULL, NULL) + ); + + } + /* For tuple insertion */ + if (buf_new) + { + /* avg =3D (mv.sum + diff.sum)::aggtype / (mv.count + diff.count) */ + appendStringInfo(buf_new, + ", %s =3D %s OPERATOR(pg_catalog./) %s", + quote_qualified_identifier(NULL, resname), + get_operation_string(IVM_ADD, sum_col, "mv", "diff", count_col, aggtype= ), + get_operation_string(IVM_ADD, count_col, "mv", "diff", NULL, NULL) + ); + /* sum =3D mv.sum + diff.sum */ + appendStringInfo(buf_new, + ", %s =3D %s", + quote_qualified_identifier(NULL, sum_col), + get_operation_string(IVM_ADD, sum_col, "mv", "diff", count_col, NULL) + ); + /* count =3D mv.count + diff.count */ + appendStringInfo(buf_new, + ", %s =3D %s", + quote_qualified_identifier(NULL, count_col), + get_operation_string(IVM_ADD, count_col, "mv", "diff", NULL, NULL) + ); + } + + appendStringInfo(aggs_list, ", %s, %s, %s", + quote_qualified_identifier("diff", resname), + quote_qualified_identifier("diff", IVM_colname("sum", resname)), + quote_qualified_identifier("diff", IVM_colname("count", resname)) + ); +} + +/* + * get_operation_string + * + * Build a string to calculate the new aggregate values. + */ +static char * +get_operation_string(IvmOp op, const char *col, const char *arg1, const ch= ar *arg2, + const char* count_col, const char *castType) +{ + StringInfoData buf; + StringInfoData castString; + char *col1 =3D quote_qualified_identifier(arg1, col); + char *col2 =3D quote_qualified_identifier(arg2, col); + char op_char =3D (op =3D=3D IVM_SUB ? '-' : '+'); + + initStringInfo(&buf); + initStringInfo(&castString); + + if (castType) + appendStringInfo(&castString, "::%s", castType); + + if (!count_col) + { + /* + * If the attributes don't have count columns then calc the result + * by using the operator simply. + */ + appendStringInfo(&buf, "(%s OPERATOR(pg_catalog.%c) %s)%s", + col1, op_char, col2, castString.data); + } + else + { + /* + * If the attributes have count columns then consider the condition + * where the result becomes NULL. + */ + char *null_cond =3D get_null_condition_string(op, arg1, arg2, count_col); + + appendStringInfo(&buf, + "(CASE WHEN %s THEN NULL " + "WHEN %s IS NULL THEN %s " + "WHEN %s IS NULL THEN %s " + "ELSE (%s OPERATOR(pg_catalog.%c) %s)%s END)", + null_cond, + col1, col2, + col2, col1, + col1, op_char, col2, castString.data + ); + } + + return buf.data; +} + +/* + * get_null_condition_string + * + * Build a predicate string for CASE clause to check if an aggregate value + * will became NULL after the given operation is applied. + */ +static char * +get_null_condition_string(IvmOp op, const char *arg1, const char *arg2, + const char* count_col) +{ + StringInfoData null_cond; + initStringInfo(&null_cond); + + switch (op) + { + case IVM_ADD: + appendStringInfo(&null_cond, + "%s OPERATOR(pg_catalog.=3D) 0 AND %s OPERATOR(pg_catalog.=3D) 0", + quote_qualified_identifier(arg1, count_col), + quote_qualified_identifier(arg2, count_col) + ); + break; + case IVM_SUB: + appendStringInfo(&null_cond, + "%s OPERATOR(pg_catalog.=3D) %s", + quote_qualified_identifier(arg1, count_col), + quote_qualified_identifier(arg2, count_col) + ); + break; + default: + elog(ERROR,"unknown operation"); + } + + return null_cond.data; +} + + /* * apply_old_delta_with_count * @@ -2143,13 +2522,20 @@ apply_delta(Oid matviewOid, Tuplestorestate *old_tu= plestores, Tuplestorestate *n * which contains tuples to be deleted from to a materialized view given by * matviewname. This is used when counting is required, that is, the view * has aggregate or distinct. + * + * If the view desn't have aggregates or has GROUP BY, this requires a keys + * list to identify a tuple in the view. If the view has aggregates, this + * requires strings representing resnames of aggregates and SET clause for + * updating aggregate values. */ static void apply_old_delta_with_count(const char *matviewname, const char *deltaname_= old, - List *keys, const char *count_colname) + List *keys, StringInfo aggs_list, StringInfo aggs_set, + const char *count_colname) { StringInfoData querybuf; char *match_cond; + bool agg_without_groupby =3D (list_length(keys) =3D=3D 0); =20 /* build WHERE condition for searching tuples to be deleted */ match_cond =3D get_matching_condition_string(keys); @@ -2159,22 +2545,26 @@ apply_old_delta_with_count(const char *matviewname,= const char *deltaname_old, appendStringInfo(&querybuf, "WITH t AS (" /* collecting tid of target tuples in the view */ "SELECT diff.%s, " /* count column */ - "(diff.%s OPERATOR(pg_catalog.=3D) mv.%s) AS for_dlt, " + "(diff.%s OPERATOR(pg_catalog.=3D) mv.%s AND %s) AS for_dlt, " "mv.ctid " + "%s " /* aggregate columns */ "FROM %s AS mv, %s AS diff " "WHERE %s" /* tuple matching condition */ "), updt AS (" /* update a tuple if this is not to be deleted */ "UPDATE %s AS mv SET %s =3D mv.%s OPERATOR(pg_catalog.-) t.%s " + "%s" /* SET clauses for aggregates */ "FROM t WHERE mv.ctid OPERATOR(pg_catalog.=3D) t.ctid AND NOT for_dl= t " ")" /* delete a tuple if this is to be deleted */ "DELETE FROM %s AS mv USING t " "WHERE mv.ctid OPERATOR(pg_catalog.=3D) t.ctid AND for_dlt", count_colname, - count_colname, count_colname, + count_colname, count_colname, (agg_without_groupby ? "false" : "true"= ), + (aggs_list !=3D NULL ? aggs_list->data : ""), matviewname, deltaname_old, match_cond, matviewname, count_colname, count_colname, count_colname, + (aggs_set !=3D NULL ? aggs_set->data : ""), matviewname); =20 if (SPI_exec(querybuf.data, 0) !=3D SPI_OK_DELETE) @@ -2238,10 +2628,15 @@ apply_old_delta(const char *matviewname, const char= *deltaname_old, * matviewname. This is used when counting is required, that is, the view * has aggregate or distinct. Also, when a table in EXISTS sub queries * is modified. + * + * If the view desn't have aggregates or has GROUP BY, this requires a keys + * list to identify a tuple in the view. If the view has aggregates, this + * requires strings representing SET clause for updating aggregate values. */ static void apply_new_delta_with_count(const char *matviewname, const char* deltaname_= new, - List *keys, StringInfo target_list, const char* count_colname) + List *keys, StringInfo aggs_set, StringInfo target_list, + const char* count_colname) { StringInfoData querybuf; StringInfoData returning_keys; @@ -2272,6 +2667,7 @@ apply_new_delta_with_count(const char *matviewname, c= onst char* deltaname_new, appendStringInfo(&querybuf, "WITH updt AS (" /* update a tuple if this exists in the view */ "UPDATE %s AS mv SET %s =3D mv.%s OPERATOR(pg_catalog.+) diff.%s " + "%s " /* SET clauses for aggregates */ "FROM %s AS diff " "WHERE %s " /* tuple matching condition */ "RETURNING %s" /* returning keys of updated tuples */ @@ -2279,6 +2675,7 @@ apply_new_delta_with_count(const char *matviewname, c= onst char* deltaname_new, "SELECT %s FROM %s AS diff " "WHERE NOT EXISTS (SELECT 1 FROM updt AS mv WHERE %s);", matviewname, count_colname, count_colname, count_colname, + (aggs_set !=3D NULL ? aggs_set->data : ""), deltaname_new, match_cond, returning_keys.data, diff --git a/src/include/commands/createas.h b/src/include/commands/createa= s.h index 76a7873ebf..599bae3b5a 100644 --- a/src/include/commands/createas.h +++ b/src/include/commands/createas.h @@ -30,6 +30,7 @@ extern void CreateIvmTriggersOnBaseTables(Query *qry, Oid= matviewOid); extern void CreateIndexOnIMMV(Query *query, Relation matviewRel); =20 extern Query *rewriteQueryForIMMV(Query *query, List *colNames); +extern void makeIvmAggColumn(ParseState *pstate, Aggref *aggref, char *res= name, AttrNumber *next_resno, List **aggs); =20 extern int GetIntoRelEFlags(IntoClause *intoClause); =20 --=20 2.25.1 --Multipart=_Thu__1_Jun_2023_23_59_09_+0900_/G5+8nG46.f1T42K Content-Type: text/x-diff; name="v28-0009-Add-support-for-min-max-aggregates-for-IVM.patch" Content-Disposition: attachment; filename="v28-0009-Add-support-for-min-max-aggregates-for-IVM.patch" Content-Transfer-Encoding: 7bit ^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: [PATCH] Expose checkpoint reason to completion log messages. @ 2026-01-08 06:28 Soumya S Murali <[email protected]> 0 siblings, 3 replies; 13+ messages in thread From: Soumya S Murali @ 2026-01-08 06:28 UTC (permalink / raw) To: Michael Banck <[email protected]>; +Cc: Fujii Masao <[email protected]>; VASUKI M <[email protected]>; Andres Freund <[email protected]>; PostgreSQL Hackers <[email protected]>; Álvaro Herrera <[email protected]>; [email protected]; [email protected] Hi all, On Wed, Jan 7, 2026 at 1:37 PM Michael Banck <[email protected]> wrote: > > Hi, > > On Wed, Jan 07, 2026 at 12:35:08PM +0530, Soumya S Murali wrote: > > I agree with your point that the checkpoint completion message should > > use the same terminology as the starting message ( “fast” rather than > > “immediate”) and I have updated the patch accordingly and have > > attached herewith for review. This patch aligns the checkpoint > > completion log wording with the existing checkpoint starting message > > (using “fast” instead of “immediate”) for consistency. > > First off, please send complete and not incremental patches. > > From what I can tell, you currently also do not have "wal" as reason? > Also, you have "timed" vs "time" etc. > > I think it would make most sense to factor out the logic for the > starting checkpoint reason from xlog.c ("(errmsg("checkpoint > starting:%s%s%s%s%s%s%s%s")") and use that both for starting and > stopping. > > > Michael Thank you for the review and feedback. I have reworked the patch to factor out the checkpoint reason logic to use the same wording for both checkpoint start and completion log (including wal, time, fast, force, and wait). The format has been aligned with existing conventions and has been verified by running make check and recovery TAP tests including t/019_replslot_limit.pl successfully. Kindly review my patch and let me know the feedback. Regards, Soumya Attachments: [text/x-patch] 0001-Expose-checkpoint-reason-in-checkpoint-completion-lo.patch (4.2K, ../../CAMtXxw9qg+-G24Xe-P=a2gWmCGnE4-CcOBOZ9tdO9eAqjMgEvQ@mail.gmail.com/2-0001-Expose-checkpoint-reason-in-checkpoint-completion-lo.patch) download | inline diff: From 0204328b225d5641a13e3e6f4c373622df31a398 Mon Sep 17 00:00:00 2001 From: Soumya <[email protected]> Date: Thu, 8 Jan 2026 11:42:23 +0530 Subject: [PATCH] Expose checkpoint reason in checkpoint completion log messages Signed-off-by: Soumya <[email protected]> --- src/backend/access/transam/xlog.c | 51 ++++++++++++++++++----- src/test/recovery/t/019_replslot_limit.pl | 2 +- 2 files changed, 41 insertions(+), 12 deletions(-) diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 22d0a2e8c3..9f7439f9a2 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -203,6 +203,8 @@ const struct config_enum_entry archive_mode_options[] = { {NULL, 0, false} }; +static char checkpoint_reason_str[128]; + /* * Statistics for current checkpoint are collected in this global struct. * Because only the checkpointer or a stand-alone backend can perform @@ -6714,12 +6716,47 @@ ShutdownXLOG(int code, Datum arg) } } +/* + * Format checkpoint reason flags consistently for log messages. + * The returned string is suitable for inclusion after + * "checkpoint starting:" or inside "checkpoint complete (...)". + */ +static const char * +CheckpointReasonString(int flags) +{ + static char buf[128]; + + buf[0] = '\0'; + + if (flags & CHECKPOINT_IS_SHUTDOWN) + strcat(buf, " shutdown"); + if (flags & CHECKPOINT_END_OF_RECOVERY) + strcat(buf, " end-of-recovery"); + if (flags & CHECKPOINT_FAST) + strcat(buf, " fast"); + if (flags & CHECKPOINT_FORCE) + strcat(buf, " force"); + if (flags & CHECKPOINT_WAIT) + strcat(buf, " wait"); + if (flags & CHECKPOINT_CAUSE_XLOG) + strcat(buf, " wal"); + if (flags & CHECKPOINT_CAUSE_TIME) + strcat(buf, " time"); + if (flags & CHECKPOINT_FLUSH_UNLOGGED) + strcat(buf, " flush-unlogged"); + + return buf; +} + /* * Log start of a checkpoint. */ static void LogCheckpointStart(int flags, bool restartpoint) { + strlcpy(checkpoint_reason_str, + CheckpointReasonString(flags), + sizeof(checkpoint_reason_str)); if (restartpoint) ereport(LOG, /* translator: the placeholders show checkpoint options */ @@ -6734,16 +6771,7 @@ LogCheckpointStart(int flags, bool restartpoint) (flags & CHECKPOINT_FLUSH_UNLOGGED) ? " flush-unlogged" : ""))); else ereport(LOG, - /* translator: the placeholders show checkpoint options */ - (errmsg("checkpoint starting:%s%s%s%s%s%s%s%s", - (flags & CHECKPOINT_IS_SHUTDOWN) ? " shutdown" : "", - (flags & CHECKPOINT_END_OF_RECOVERY) ? " end-of-recovery" : "", - (flags & CHECKPOINT_FAST) ? " fast" : "", - (flags & CHECKPOINT_FORCE) ? " force" : "", - (flags & CHECKPOINT_WAIT) ? " wait" : "", - (flags & CHECKPOINT_CAUSE_XLOG) ? " wal" : "", - (flags & CHECKPOINT_CAUSE_TIME) ? " time" : "", - (flags & CHECKPOINT_FLUSH_UNLOGGED) ? " flush-unlogged" : ""))); + (errmsg("checkpoint starting:%s", checkpoint_reason_str))); } /* @@ -6824,12 +6852,13 @@ LogCheckpointEnd(bool restartpoint) LSN_FORMAT_ARGS(ControlFile->checkPointCopy.redo)))); else ereport(LOG, - (errmsg("checkpoint complete: wrote %d buffers (%.1f%%), " + (errmsg("checkpoint complete (%s): wrote %d buffers (%.1f%%), " "wrote %d SLRU buffers; %d WAL file(s) added, " "%d removed, %d recycled; write=%ld.%03d s, " "sync=%ld.%03d s, total=%ld.%03d s; sync files=%d, " "longest=%ld.%03d s, average=%ld.%03d s; distance=%d kB, " "estimate=%d kB; lsn=%X/%08X, redo lsn=%X/%08X", + checkpoint_reason_str, CheckpointStats.ckpt_bufs_written, (double) CheckpointStats.ckpt_bufs_written * 100 / NBuffers, CheckpointStats.ckpt_slru_written, diff --git a/src/test/recovery/t/019_replslot_limit.pl b/src/test/recovery/t/019_replslot_limit.pl index 6468784b83..ccc363157e 100644 --- a/src/test/recovery/t/019_replslot_limit.pl +++ b/src/test/recovery/t/019_replslot_limit.pl @@ -203,7 +203,7 @@ is($result, "rep1|f|t|lost|", my $checkpoint_ended = 0; for (my $i = 0; $i < 10 * $PostgreSQL::Test::Utils::timeout_default; $i++) { - if ($node_primary->log_contains("checkpoint complete: ", $logstart)) + if ($node_primary->log_contains(qr/checkpoint complete(?: \([^)]*\))?:/, $logstart)) { $checkpoint_ended = 1; last; -- 2.34.1 ^ permalink raw reply [nested|flat] 13+ messages in thread
* 回复: [PATCH] Expose checkpoint reason to completion log messages. @ 2026-01-08 13:31 li carol <[email protected]> parent: Soumya S Murali <[email protected]> 2 siblings, 0 replies; 13+ messages in thread From: li carol @ 2026-01-08 13:31 UTC (permalink / raw) To: Soumya S Murali <[email protected]>; Michael Banck <[email protected]>; +Cc: Fujii Masao <[email protected]>; VASUKI M <[email protected]>; Andres Freund <[email protected]>; PostgreSQL Hackers <[email protected]>; Álvaro Herrera <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]> > > Hi all, > > On Wed, Jan 7, 2026 at 1:37 PM Michael Banck <[email protected]> wrote: > > > > Hi, > > > > On Wed, Jan 07, 2026 at 12:35:08PM +0530, Soumya S Murali wrote: > > > I agree with your point that the checkpoint completion message > > > should use the same terminology as the starting message ( “fast” > > > rather than > > > “immediate”) and I have updated the patch accordingly and have > > > attached herewith for review. This patch aligns the checkpoint > > > completion log wording with the existing checkpoint starting message > > > (using “fast” instead of “immediate”) for consistency. > > > > First off, please send complete and not incremental patches. > > > > From what I can tell, you currently also do not have "wal" as reason? > > Also, you have "timed" vs "time" etc. > > > > I think it would make most sense to factor out the logic for the > > starting checkpoint reason from xlog.c ("(errmsg("checkpoint > > starting:%s%s%s%s%s%s%s%s")") and use that both for starting and > > stopping. > > > > > > Michael > > Thank you for the review and feedback. > I have reworked the patch to factor out the checkpoint reason logic to use the > same wording for both checkpoint start and completion log (including wal, time, > fast, force, and wait). The format has been aligned with existing conventions > and has been verified by running make check and recovery TAP tests including > t/019_replslot_limit.pl successfully. > Kindly review my patch and let me know the feedback. > > Regards, > Soumya Hi Soumya, Thanks for the updated patch. I agree that having the checkpoint reason in the completion log is very helpful for performance analysis, as it avoids the need to manually correlate "starting" and "complete" messages in busy logs. I’ve reviewed the latest version and have a few suggestions to make the refactoring more complete: 1. Consistency in LogCheckpointStart Currently, the checkpoint branch uses the new CheckpointReasonString helper, but the restartpoint branch still uses the old inline triple-operator logic. It would be better to use the helper function in both places to ensure they stay in sync. 2. Missing Reason for Restartpoints In LogCheckpointEnd, the checkpoint reason is added to the checkpoint complete message, but the restartpoint complete message remains unchanged. I think users would find the reason for restartpoints just as useful for debugging. 3. Global Variable vs. Passing Flags: Instead of using a file-level global checkpoint_reason_str to pass data between the start and end functions, would it make sense to simply pass the flags as a new argument to LogCheckpointEnd(bool restartpoint, int flags)? This would allow you to call the helper function directly inside the ereport calls and avoid maintaining global state. 4. Whitespace Errors: I noticed a few whitespace warnings (space before tabs) when applying the patch. /home/carol/0001-Expose-checkpoint-reason-in-checkpoint-completion-lo.patch:69: space before tab in indent. CheckpointReasonString(flags), /home/carol/0001-Expose-checkpoint-reason-in-checkpoint-completion-lo.patch:70: space before tab in indent. sizeof(checkpoint_reason_str)); /home/carol/0001-Expose-checkpoint-reason-in-checkpoint-completion-lo.patch:88: space before tab in indent. (errmsg("checkpoint starting:%s", checkpoint_reason_str))); warning: 3 lines add whitespace errors. The patch can be compiled successfully and can pass make check -C src/test/recovery/ Regards, Yuan Li(carol) ^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: [PATCH] Expose checkpoint reason to completion log messages. @ 2026-01-08 17:36 Michael Banck <[email protected]> parent: Soumya S Murali <[email protected]> 2 siblings, 0 replies; 13+ messages in thread From: Michael Banck @ 2026-01-08 17:36 UTC (permalink / raw) To: Soumya S Murali <[email protected]>; +Cc: Fujii Masao <[email protected]>; VASUKI M <[email protected]>; Andres Freund <[email protected]>; PostgreSQL Hackers <[email protected]>; Álvaro Herrera <[email protected]>; [email protected]; [email protected] Hi, On Thu, Jan 08, 2026 at 11:58:43AM +0530, Soumya S Murali wrote: > On Wed, Jan 7, 2026 at 1:37 PM Michael Banck <[email protected]> wrote: > > On Wed, Jan 07, 2026 at 12:35:08PM +0530, Soumya S Murali wrote: > I have reworked the patch to factor out the checkpoint reason logic to > use the same wording for both checkpoint start and completion log > (including wal, time, fast, force, and wait). The format has been > aligned with existing conventions and has been verified by running > make check and recovery TAP tests including t/019_replslot_limit.pl > successfully. > Kindly review my patch and let me know the feedback. Thanks for the new patch. > index 22d0a2e8c3..9f7439f9a2 100644 > --- a/src/backend/access/transam/xlog.c > +++ b/src/backend/access/transam/xlog.c [...] > +static const char * > +CheckpointReasonString(int flags) > +{ > + static char buf[128]; > + > + buf[0] = '\0'; > + > + if (flags & CHECKPOINT_IS_SHUTDOWN) > + strcat(buf, " shutdown"); > + if (flags & CHECKPOINT_END_OF_RECOVERY) > + strcat(buf, " end-of-recovery"); > + if (flags & CHECKPOINT_FAST) > + strcat(buf, " fast"); > + if (flags & CHECKPOINT_FORCE) > + strcat(buf, " force"); > + if (flags & CHECKPOINT_WAIT) > + strcat(buf, " wait"); > + if (flags & CHECKPOINT_CAUSE_XLOG) > + strcat(buf, " wal"); > + if (flags & CHECKPOINT_CAUSE_TIME) > + strcat(buf, " time"); > + if (flags & CHECKPOINT_FLUSH_UNLOGGED) > + strcat(buf, " flush-unlogged"); > + > + return buf; > +} The way this is coded now prepends the final string with a space character. This is fine for the checkpoint starting message, but makes the checkpoint finished message look slightly weird (not the "( wal)"): |2026-01-08 18:30:33.691 CET [1666416] LOG: checkpoint complete ( wal): |wrote 20 buffers (0.1%), wrote 0 SLRU buffers; 0 WAL file(s) added, 4 |removed, 33 recycled; write=2.221 s, sync=0.019 s, total=2.701 s; sync |files=3, longest=0.018 s, average=0.007 s; distance=601885 kB, |estimate=601885 kB; lsn=0/D2D59FC8, redo lsn=0/B492B500 Also I think the translators might now complain, but I am not sure how to best approach this. Michael ^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: [PATCH] Expose checkpoint reason to completion log messages. @ 2026-01-12 07:23 Soumya S Murali <[email protected]> parent: Soumya S Murali <[email protected]> 2 siblings, 2 replies; 13+ messages in thread From: Soumya S Murali @ 2026-01-12 07:23 UTC (permalink / raw) To: Michael Banck <[email protected]>; +Cc: Fujii Masao <[email protected]>; VASUKI M <[email protected]>; Andres Freund <[email protected]>; PostgreSQL Hackers <[email protected]>; Álvaro Herrera <[email protected]>; [email protected]; [email protected] Hi all, On Thu, Jan 8, 2026 at 11:06 PM Michael Banck <[email protected]> wrote: > > Hi, > > On Thu, Jan 08, 2026 at 11:58:43AM +0530, Soumya S Murali wrote: > > On Wed, Jan 7, 2026 at 1:37 PM Michael Banck <[email protected]> wrote: > > > On Wed, Jan 07, 2026 at 12:35:08PM +0530, Soumya S Murali wrote: > > I have reworked the patch to factor out the checkpoint reason logic to > > use the same wording for both checkpoint start and completion log > > (including wal, time, fast, force, and wait). The format has been > > aligned with existing conventions and has been verified by running > > make check and recovery TAP tests including t/019_replslot_limit.pl > > successfully. > > Kindly review my patch and let me know the feedback. > > Thanks for the new patch. > > > index 22d0a2e8c3..9f7439f9a2 100644 > > --- a/src/backend/access/transam/xlog.c > > +++ b/src/backend/access/transam/xlog.c > [...] > > +static const char * > > +CheckpointReasonString(int flags) > > +{ > > + static char buf[128]; > > + > > + buf[0] = '\0'; > > + > > + if (flags & CHECKPOINT_IS_SHUTDOWN) > > + strcat(buf, " shutdown"); > > + if (flags & CHECKPOINT_END_OF_RECOVERY) > > + strcat(buf, " end-of-recovery"); > > + if (flags & CHECKPOINT_FAST) > > + strcat(buf, " fast"); > > + if (flags & CHECKPOINT_FORCE) > > + strcat(buf, " force"); > > + if (flags & CHECKPOINT_WAIT) > > + strcat(buf, " wait"); > > + if (flags & CHECKPOINT_CAUSE_XLOG) > > + strcat(buf, " wal"); > > + if (flags & CHECKPOINT_CAUSE_TIME) > > + strcat(buf, " time"); > > + if (flags & CHECKPOINT_FLUSH_UNLOGGED) > > + strcat(buf, " flush-unlogged"); > > + > > + return buf; > > +} > > The way this is coded now prepends the final string with a space > character. This is fine for the checkpoint starting message, but makes > the checkpoint finished message look slightly weird (not the "( wal)"): > > |2026-01-08 18:30:33.691 CET [1666416] LOG: checkpoint complete ( wal): > |wrote 20 buffers (0.1%), wrote 0 SLRU buffers; 0 WAL file(s) added, 4 > |removed, 33 recycled; write=2.221 s, sync=0.019 s, total=2.701 s; sync > |files=3, longest=0.018 s, average=0.007 s; distance=601885 kB, > |estimate=601885 kB; lsn=0/D2D59FC8, redo lsn=0/B492B500 > > Also I think the translators might now complain, but I am not sure how > to best approach this. Based on the feedback received, I have reworked and implemented a helper function so that both checkpoint start and completion messages use the same wording and would remain in sync, extending the same reason reporting to restart points for completeness and better observability, ensuring consistent terminology with existing log output, had removed the previous use of global state and instead passed checkpoint flags explicitly and have fixed formatting and whitespace issues noted during review. Also, I have tested and validated the logic successfully. I have attached the updated patch for further review. Kindly review the patch and let me know the feedback. Regards, Soumya Attachments: [text/x-patch] 0001-Expose-checkpoint-reason-in-completion-log-messages.patch (5.9K, ../../CAMtXxw_o3TTaUiTqqwMCEMtLNESX9-Dvd60=CkgQO37KW1AkXQ@mail.gmail.com/2-0001-Expose-checkpoint-reason-in-completion-log-messages.patch) download | inline diff: From 30e3c62695a643daf96ffca3b504a79d2a761f77 Mon Sep 17 00:00:00 2001 From: Soumya <[email protected]> Date: Mon, 12 Jan 2026 12:36:29 +0530 Subject: [PATCH] Expose checkpoint reason in completion log messages --- src/backend/access/transam/xlog.c | 78 ++++++++++++++++------- src/test/recovery/t/019_replslot_limit.pl | 2 +- 2 files changed, 55 insertions(+), 25 deletions(-) diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 22d0a2e8c3..63d1e56ca1 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -6714,6 +6714,49 @@ ShutdownXLOG(int code, Datum arg) } } +/* +* Format checkpoint reason flags consistently for log messages. +* The returned string is suitable for inclusion after +* "checkpoint starting:" or inside "checkpoint complete (...)". +*/ +static const char * +CheckpointReasonString(int flags) +{ + static char buf[128]; + bool first = true; + + buf[0] = '\0'; + +#define APPEND_REASON(str) \ + do { \ + if (!first) \ + strcat(buf, " "); \ + strcat(buf, (str)); \ + first = false; \ + } while (0) + + if (flags & CHECKPOINT_IS_SHUTDOWN) + APPEND_REASON("shutdown"); + if (flags & CHECKPOINT_END_OF_RECOVERY) + APPEND_REASON("end-of-recovery"); + if (flags & CHECKPOINT_FAST) + APPEND_REASON("fast"); + if (flags & CHECKPOINT_FORCE) + APPEND_REASON("force"); + if (flags & CHECKPOINT_WAIT) + APPEND_REASON("wait"); + if (flags & CHECKPOINT_CAUSE_XLOG) + APPEND_REASON("wal"); + if (flags & CHECKPOINT_CAUSE_TIME) + APPEND_REASON("time"); + if (flags & CHECKPOINT_FLUSH_UNLOGGED) + APPEND_REASON("flush-unlogged"); + +#undef APPEND_REASON + + return buf; +} + /* * Log start of a checkpoint. */ @@ -6723,34 +6766,19 @@ LogCheckpointStart(int flags, bool restartpoint) if (restartpoint) ereport(LOG, /* translator: the placeholders show checkpoint options */ - (errmsg("restartpoint starting:%s%s%s%s%s%s%s%s", - (flags & CHECKPOINT_IS_SHUTDOWN) ? " shutdown" : "", - (flags & CHECKPOINT_END_OF_RECOVERY) ? " end-of-recovery" : "", - (flags & CHECKPOINT_FAST) ? " fast" : "", - (flags & CHECKPOINT_FORCE) ? " force" : "", - (flags & CHECKPOINT_WAIT) ? " wait" : "", - (flags & CHECKPOINT_CAUSE_XLOG) ? " wal" : "", - (flags & CHECKPOINT_CAUSE_TIME) ? " time" : "", - (flags & CHECKPOINT_FLUSH_UNLOGGED) ? " flush-unlogged" : ""))); + (errmsg("restartpoint starting: %s", + CheckpointReasonString(flags)))); else ereport(LOG, - /* translator: the placeholders show checkpoint options */ - (errmsg("checkpoint starting:%s%s%s%s%s%s%s%s", - (flags & CHECKPOINT_IS_SHUTDOWN) ? " shutdown" : "", - (flags & CHECKPOINT_END_OF_RECOVERY) ? " end-of-recovery" : "", - (flags & CHECKPOINT_FAST) ? " fast" : "", - (flags & CHECKPOINT_FORCE) ? " force" : "", - (flags & CHECKPOINT_WAIT) ? " wait" : "", - (flags & CHECKPOINT_CAUSE_XLOG) ? " wal" : "", - (flags & CHECKPOINT_CAUSE_TIME) ? " time" : "", - (flags & CHECKPOINT_FLUSH_UNLOGGED) ? " flush-unlogged" : ""))); + (errmsg("checkpoint starting: %s", + CheckpointReasonString(flags)))); } /* * Log end of a checkpoint. */ static void -LogCheckpointEnd(bool restartpoint) +LogCheckpointEnd(bool restartpoint, int flags) { long write_msecs, sync_msecs, @@ -6800,12 +6828,13 @@ LogCheckpointEnd(bool restartpoint) */ if (restartpoint) ereport(LOG, - (errmsg("restartpoint complete: wrote %d buffers (%.1f%%), " + (errmsg("restartpoint complete (%s): wrote %d buffers (%.1f%%), " "wrote %d SLRU buffers; %d WAL file(s) added, " "%d removed, %d recycled; write=%ld.%03d s, " "sync=%ld.%03d s, total=%ld.%03d s; sync files=%d, " "longest=%ld.%03d s, average=%ld.%03d s; distance=%d kB, " "estimate=%d kB; lsn=%X/%08X, redo lsn=%X/%08X", + CheckpointReasonString(flags), CheckpointStats.ckpt_bufs_written, (double) CheckpointStats.ckpt_bufs_written * 100 / NBuffers, CheckpointStats.ckpt_slru_written, @@ -6824,12 +6853,13 @@ LogCheckpointEnd(bool restartpoint) LSN_FORMAT_ARGS(ControlFile->checkPointCopy.redo)))); else ereport(LOG, - (errmsg("checkpoint complete: wrote %d buffers (%.1f%%), " + (errmsg("checkpoint complete (%s): wrote %d buffers (%.1f%%), " "wrote %d SLRU buffers; %d WAL file(s) added, " "%d removed, %d recycled; write=%ld.%03d s, " "sync=%ld.%03d s, total=%ld.%03d s; sync files=%d, " "longest=%ld.%03d s, average=%ld.%03d s; distance=%d kB, " "estimate=%d kB; lsn=%X/%08X, redo lsn=%X/%08X", + CheckpointReasonString(flags), CheckpointStats.ckpt_bufs_written, (double) CheckpointStats.ckpt_bufs_written * 100 / NBuffers, CheckpointStats.ckpt_slru_written, @@ -7418,7 +7448,7 @@ CreateCheckPoint(int flags) TruncateSUBTRANS(GetOldestTransactionIdConsideredRunning()); /* Real work is done; log and update stats. */ - LogCheckpointEnd(false); + LogCheckpointEnd(false, flags); /* Reset the process title */ update_checkpoint_display(flags, false, true); @@ -7886,7 +7916,7 @@ CreateRestartPoint(int flags) TruncateSUBTRANS(GetOldestTransactionIdConsideredRunning()); /* Real work is done; log and update stats. */ - LogCheckpointEnd(true); + LogCheckpointEnd(true, flags); /* Reset the process title */ update_checkpoint_display(flags, true, true); diff --git a/src/test/recovery/t/019_replslot_limit.pl b/src/test/recovery/t/019_replslot_limit.pl index 6468784b83..ccc363157e 100644 --- a/src/test/recovery/t/019_replslot_limit.pl +++ b/src/test/recovery/t/019_replslot_limit.pl @@ -203,7 +203,7 @@ is($result, "rep1|f|t|lost|", my $checkpoint_ended = 0; for (my $i = 0; $i < 10 * $PostgreSQL::Test::Utils::timeout_default; $i++) { - if ($node_primary->log_contains("checkpoint complete: ", $logstart)) + if ($node_primary->log_contains(qr/checkpoint complete(?: \([^)]*\))?:/, $logstart)) { $checkpoint_ended = 1; last; -- 2.34.1 ^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: [PATCH] Expose checkpoint reason to completion log messages. @ 2026-01-12 10:26 Michael Banck <[email protected]> parent: Soumya S Murali <[email protected]> 1 sibling, 0 replies; 13+ messages in thread From: Michael Banck @ 2026-01-12 10:26 UTC (permalink / raw) To: Soumya S Murali <[email protected]>; +Cc: Fujii Masao <[email protected]>; VASUKI M <[email protected]>; Andres Freund <[email protected]>; PostgreSQL Hackers <[email protected]>; Álvaro Herrera <[email protected]>; [email protected]; [email protected] Hi, On Mon, Jan 12, 2026 at 12:53:54PM +0530, Soumya S Murali wrote: > Based on the feedback received, I have reworked and implemented a > helper function so that both checkpoint start and completion messages > use the same wording and would remain in sync, extending the same > reason reporting to restart points for completeness and better > observability, ensuring consistent terminology with existing log > output, had removed the previous use of global state and instead > passed checkpoint flags explicitly and have fixed formatting and > whitespace issues noted during review. > Also, I have tested and validated the logic successfully. I have > attached the updated patch for further review. > Kindly review the patch and let me know the feedback. > > Regards, > Soumya > From 30e3c62695a643daf96ffca3b504a79d2a761f77 Mon Sep 17 00:00:00 2001 > From: Soumya <[email protected]> > Date: Mon, 12 Jan 2026 12:36:29 +0530 > Subject: [PATCH] Expose checkpoint reason in completion log messages > > --- > src/backend/access/transam/xlog.c | 78 ++++++++++++++++------- > src/test/recovery/t/019_replslot_limit.pl | 2 +- > 2 files changed, 55 insertions(+), 25 deletions(-) > > diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c > index 22d0a2e8c3..63d1e56ca1 100644 > --- a/src/backend/access/transam/xlog.c > +++ b/src/backend/access/transam/xlog.c > @@ -6714,6 +6714,49 @@ ShutdownXLOG(int code, Datum arg) > } > } > > +/* > +* Format checkpoint reason flags consistently for log messages. > +* The returned string is suitable for inclusion after > +* "checkpoint starting:" or inside "checkpoint complete (...)". > +*/ Indentation of the comment is wrong here, the asterisks should be all on the same column. > +static const char * > +CheckpointReasonString(int flags) > +{ > + static char buf[128]; > + bool first = true; pgindent complains about the above line as well: | { | static char buf[128]; |- bool first = true; |+ bool first = true; | | buf[0] = '\0'; > + buf[0] = '\0'; > + > +#define APPEND_REASON(str) \ > + do { \ > + if (!first) \ > + strcat(buf, " "); \ > + strcat(buf, (str)); \ > + first = false; \ > + } while (0) Those ending backslashes also don't seem to line up when 4-space tabs are set. But pgindent doesn't change them so maybe it is correct as-is? I am also not sure whether the above is current best-practise for Postgres code, I'd have to look deeper (or maybe somebody else can review this). > + if (flags & CHECKPOINT_IS_SHUTDOWN) > + APPEND_REASON("shutdown"); > + if (flags & CHECKPOINT_END_OF_RECOVERY) > + APPEND_REASON("end-of-recovery"); > + if (flags & CHECKPOINT_FAST) > + APPEND_REASON("fast"); > + if (flags & CHECKPOINT_FORCE) > + APPEND_REASON("force"); > + if (flags & CHECKPOINT_WAIT) > + APPEND_REASON("wait"); > + if (flags & CHECKPOINT_CAUSE_XLOG) > + APPEND_REASON("wal"); > + if (flags & CHECKPOINT_CAUSE_TIME) > + APPEND_REASON("time"); > + if (flags & CHECKPOINT_FLUSH_UNLOGGED) > + APPEND_REASON("flush-unlogged"); > + > +#undef APPEND_REASON > + > + return buf; I am still not sure whether this will be a regression for translation of those strings. Otherwise, the patch looks sane to me. Michael ^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: [PATCH] Expose checkpoint reason to completion log messages. @ 2026-01-13 06:29 Soumya S Murali <[email protected]> parent: Soumya S Murali <[email protected]> 1 sibling, 1 reply; 13+ messages in thread From: Soumya S Murali @ 2026-01-13 06:29 UTC (permalink / raw) To: Michael Banck <[email protected]>; +Cc: Fujii Masao <[email protected]>; VASUKI M <[email protected]>; Andres Freund <[email protected]>; PostgreSQL Hackers <[email protected]>; Álvaro Herrera <[email protected]>; [email protected]; [email protected] Hi all, On Mon, Jan 12, 2026 at 3:57 PM Michael Banck <[email protected]> wrote: > > Hi, > > On Mon, Jan 12, 2026 at 12:53:54PM +0530, Soumya S Murali wrote: > > Based on the feedback received, I have reworked and implemented a > > helper function so that both checkpoint start and completion messages > > use the same wording and would remain in sync, extending the same > > reason reporting to restart points for completeness and better > > observability, ensuring consistent terminology with existing log > > output, had removed the previous use of global state and instead > > passed checkpoint flags explicitly and have fixed formatting and > > whitespace issues noted during review. > > Also, I have tested and validated the logic successfully. I have > > attached the updated patch for further review. > > Kindly review the patch and let me know the feedback. > > > > Regards, > > Soumya > > > From 30e3c62695a643daf96ffca3b504a79d2a761f77 Mon Sep 17 00:00:00 2001 > > From: Soumya <[email protected]> > > Date: Mon, 12 Jan 2026 12:36:29 +0530 > > Subject: [PATCH] Expose checkpoint reason in completion log messages > > > > --- > > src/backend/access/transam/xlog.c | 78 ++++++++++++++++------- > > src/test/recovery/t/019_replslot_limit.pl | 2 +- > > 2 files changed, 55 insertions(+), 25 deletions(-) > > > > diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c > > index 22d0a2e8c3..63d1e56ca1 100644 > > --- a/src/backend/access/transam/xlog.c > > +++ b/src/backend/access/transam/xlog.c > > @@ -6714,6 +6714,49 @@ ShutdownXLOG(int code, Datum arg) > > } > > } > > > > +/* > > +* Format checkpoint reason flags consistently for log messages. > > +* The returned string is suitable for inclusion after > > +* "checkpoint starting:" or inside "checkpoint complete (...)". > > +*/ > > Indentation of the comment is wrong here, the asterisks should be all on > the same column. > > +static const char * > > +CheckpointReasonString(int flags) > > +{ > > + static char buf[128]; > > + bool first = true; > > pgindent complains about the above line as well: > > | { > | static char buf[128]; > |- bool first = true; > |+ bool first = true; > | > | buf[0] = '\0'; > > > + buf[0] = '\0'; > > + > > +#define APPEND_REASON(str) \ > > + do { \ > > + if (!first) \ > > + strcat(buf, " "); \ > > + strcat(buf, (str)); \ > > + first = false; \ > > + } while (0) > > Those ending backslashes also don't seem to line up when 4-space tabs > are set. But pgindent doesn't change them so maybe it is correct as-is? > > I am also not sure whether the above is current best-practise for > Postgres code, I'd have to look deeper (or maybe somebody else can > review this). > > > + if (flags & CHECKPOINT_IS_SHUTDOWN) > > + APPEND_REASON("shutdown"); > > + if (flags & CHECKPOINT_END_OF_RECOVERY) > > + APPEND_REASON("end-of-recovery"); > > + if (flags & CHECKPOINT_FAST) > > + APPEND_REASON("fast"); > > + if (flags & CHECKPOINT_FORCE) > > + APPEND_REASON("force"); > > + if (flags & CHECKPOINT_WAIT) > > + APPEND_REASON("wait"); > > + if (flags & CHECKPOINT_CAUSE_XLOG) > > + APPEND_REASON("wal"); > > + if (flags & CHECKPOINT_CAUSE_TIME) > > + APPEND_REASON("time"); > > + if (flags & CHECKPOINT_FLUSH_UNLOGGED) > > + APPEND_REASON("flush-unlogged"); > > + > > +#undef APPEND_REASON > > + > > + return buf; > > I am still not sure whether this will be a regression for translation of > those strings. > > Otherwise, the patch looks sane to me. Thank you for the detailed review and suggestions. I have reworked the patch on a fresh tree, addressed the formatting and indentation issues, and aligned the helper implementation with PostgreSQL coding conventions. The updated patch has been pgindented and validated with make check and the full recovery TAP test suite. I am attaching the revised patch for further review. Regards, Soumya Attachments: [text/x-patch] 0001-Expose-checkpoint-reason-in-completion-log-messages.patch (6.2K, ../../CAMtXxw9Cefp78beLt6mEVeD7OnxTryc0H124HX_Ru7xKs+9MJQ@mail.gmail.com/2-0001-Expose-checkpoint-reason-in-completion-log-messages.patch) download | inline diff: From c2ed1eb81b38c36399b8fbec5bf09326b06fefa0 Mon Sep 17 00:00:00 2001 From: Soumya <[email protected]> Date: Tue, 13 Jan 2026 11:50:53 +0530 Subject: [PATCH] Expose checkpoint reason in completion log messages Signed-off-by: Soumya <[email protected]> --- src/backend/access/transam/xlog.c | 79 ++++++++++++++++------- src/test/recovery/t/019_replslot_limit.pl | 2 +- 2 files changed, 56 insertions(+), 25 deletions(-) diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 22d0a2e8c3..8cdf2b23c3 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -6714,6 +6714,50 @@ ShutdownXLOG(int code, Datum arg) } } +/* + * Format checkpoint reason flags consistently for log messages. + * The returned string is suitable for inclusion after + * "checkpoint starting:" or inside "checkpoint complete (...)". + */ +static const char * +CheckpointReasonString(int flags) +{ + static char buf[128]; + bool first = true; + + buf[0] = '\0'; + +#define APPEND_REASON(str) \ + do \ + { \ + if (!first) \ + strcat(buf, " "); \ + strcat(buf, (str)); \ + first = false; \ + } while (0) + + if (flags & CHECKPOINT_IS_SHUTDOWN) + APPEND_REASON("shutdown"); + if (flags & CHECKPOINT_END_OF_RECOVERY) + APPEND_REASON("end-of-recovery"); + if (flags & CHECKPOINT_FAST) + APPEND_REASON("fast"); + if (flags & CHECKPOINT_FORCE) + APPEND_REASON("force"); + if (flags & CHECKPOINT_WAIT) + APPEND_REASON("wait"); + if (flags & CHECKPOINT_CAUSE_XLOG) + APPEND_REASON("wal"); + if (flags & CHECKPOINT_CAUSE_TIME) + APPEND_REASON("time"); + if (flags & CHECKPOINT_FLUSH_UNLOGGED) + APPEND_REASON("flush-unlogged"); + +#undef APPEND_REASON + + return buf; +} + /* * Log start of a checkpoint. */ @@ -6723,34 +6767,19 @@ LogCheckpointStart(int flags, bool restartpoint) if (restartpoint) ereport(LOG, /* translator: the placeholders show checkpoint options */ - (errmsg("restartpoint starting:%s%s%s%s%s%s%s%s", - (flags & CHECKPOINT_IS_SHUTDOWN) ? " shutdown" : "", - (flags & CHECKPOINT_END_OF_RECOVERY) ? " end-of-recovery" : "", - (flags & CHECKPOINT_FAST) ? " fast" : "", - (flags & CHECKPOINT_FORCE) ? " force" : "", - (flags & CHECKPOINT_WAIT) ? " wait" : "", - (flags & CHECKPOINT_CAUSE_XLOG) ? " wal" : "", - (flags & CHECKPOINT_CAUSE_TIME) ? " time" : "", - (flags & CHECKPOINT_FLUSH_UNLOGGED) ? " flush-unlogged" : ""))); + (errmsg("restartpoint starting: %s", + CheckpointReasonString(flags)))); else ereport(LOG, - /* translator: the placeholders show checkpoint options */ - (errmsg("checkpoint starting:%s%s%s%s%s%s%s%s", - (flags & CHECKPOINT_IS_SHUTDOWN) ? " shutdown" : "", - (flags & CHECKPOINT_END_OF_RECOVERY) ? " end-of-recovery" : "", - (flags & CHECKPOINT_FAST) ? " fast" : "", - (flags & CHECKPOINT_FORCE) ? " force" : "", - (flags & CHECKPOINT_WAIT) ? " wait" : "", - (flags & CHECKPOINT_CAUSE_XLOG) ? " wal" : "", - (flags & CHECKPOINT_CAUSE_TIME) ? " time" : "", - (flags & CHECKPOINT_FLUSH_UNLOGGED) ? " flush-unlogged" : ""))); + (errmsg("checkpoint starting: %s", + CheckpointReasonString(flags)))); } /* * Log end of a checkpoint. */ static void -LogCheckpointEnd(bool restartpoint) +LogCheckpointEnd(bool restartpoint, int flags) { long write_msecs, sync_msecs, @@ -6800,12 +6829,13 @@ LogCheckpointEnd(bool restartpoint) */ if (restartpoint) ereport(LOG, - (errmsg("restartpoint complete: wrote %d buffers (%.1f%%), " + (errmsg("restartpoint complete (%s): wrote %d buffers (%.1f%%), " "wrote %d SLRU buffers; %d WAL file(s) added, " "%d removed, %d recycled; write=%ld.%03d s, " "sync=%ld.%03d s, total=%ld.%03d s; sync files=%d, " "longest=%ld.%03d s, average=%ld.%03d s; distance=%d kB, " "estimate=%d kB; lsn=%X/%08X, redo lsn=%X/%08X", + CheckpointReasonString(flags), CheckpointStats.ckpt_bufs_written, (double) CheckpointStats.ckpt_bufs_written * 100 / NBuffers, CheckpointStats.ckpt_slru_written, @@ -6824,12 +6854,13 @@ LogCheckpointEnd(bool restartpoint) LSN_FORMAT_ARGS(ControlFile->checkPointCopy.redo)))); else ereport(LOG, - (errmsg("checkpoint complete: wrote %d buffers (%.1f%%), " + (errmsg("checkpoint complete (%s): wrote %d buffers (%.1f%%), " "wrote %d SLRU buffers; %d WAL file(s) added, " "%d removed, %d recycled; write=%ld.%03d s, " "sync=%ld.%03d s, total=%ld.%03d s; sync files=%d, " "longest=%ld.%03d s, average=%ld.%03d s; distance=%d kB, " "estimate=%d kB; lsn=%X/%08X, redo lsn=%X/%08X", + CheckpointReasonString(flags), CheckpointStats.ckpt_bufs_written, (double) CheckpointStats.ckpt_bufs_written * 100 / NBuffers, CheckpointStats.ckpt_slru_written, @@ -7418,7 +7449,7 @@ CreateCheckPoint(int flags) TruncateSUBTRANS(GetOldestTransactionIdConsideredRunning()); /* Real work is done; log and update stats. */ - LogCheckpointEnd(false); + LogCheckpointEnd(false, flags); /* Reset the process title */ update_checkpoint_display(flags, false, true); @@ -7886,7 +7917,7 @@ CreateRestartPoint(int flags) TruncateSUBTRANS(GetOldestTransactionIdConsideredRunning()); /* Real work is done; log and update stats. */ - LogCheckpointEnd(true); + LogCheckpointEnd(true, flags); /* Reset the process title */ update_checkpoint_display(flags, true, true); diff --git a/src/test/recovery/t/019_replslot_limit.pl b/src/test/recovery/t/019_replslot_limit.pl index 6468784b83..ccc363157e 100644 --- a/src/test/recovery/t/019_replslot_limit.pl +++ b/src/test/recovery/t/019_replslot_limit.pl @@ -203,7 +203,7 @@ is($result, "rep1|f|t|lost|", my $checkpoint_ended = 0; for (my $i = 0; $i < 10 * $PostgreSQL::Test::Utils::timeout_default; $i++) { - if ($node_primary->log_contains("checkpoint complete: ", $logstart)) + if ($node_primary->log_contains(qr/checkpoint complete(?: \([^)]*\))?:/, $logstart)) { $checkpoint_ended = 1; last; -- 2.34.1 ^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: [PATCH] Expose checkpoint reason to completion log messages. @ 2026-01-13 13:09 Fujii Masao <[email protected]> parent: Soumya S Murali <[email protected]> 0 siblings, 1 reply; 13+ messages in thread From: Fujii Masao @ 2026-01-13 13:09 UTC (permalink / raw) To: Soumya S Murali <[email protected]>; +Cc: Michael Banck <[email protected]>; VASUKI M <[email protected]>; Andres Freund <[email protected]>; PostgreSQL Hackers <[email protected]>; Álvaro Herrera <[email protected]>; [email protected]; [email protected] On Tue, Jan 13, 2026 at 3:29 PM Soumya S Murali <[email protected]> wrote: > Thank you for the detailed review and suggestions. > I have reworked the patch on a fresh tree, addressed the formatting > and indentation issues, and aligned the helper implementation with > PostgreSQL coding conventions. The updated patch has been pgindented > and validated with make check and the full recovery TAP test suite. I > am attaching the revised patch for further review. Thanks for updating the patch! With this patch, the checkpoint log messages look like this: LOG: checkpoint starting: fast force wait LOG: checkpoint complete (fast force wait): ... The formatting of the checkpoint flags differs between the starting and complete messages: the complete message wraps them in parentheses, while the starting message does not. I think it would be better to log the flags in a consistent way in both messages. For example: LOG: checkpoint starting: fast force wait LOG: checkpoint complete: fast force wait: ... - if ($node_primary->log_contains("checkpoint complete: ", $logstart)) + if ($node_primary->log_contains(qr/checkpoint complete(?: \([^)]*\))?:/, $logstart)) If we adopt a format like the above example, this test change would no longer be needed. +#define APPEND_REASON(str) \ + do \ + { \ + if (!first) \ Wouldn't it be simpler to just build the string with snprintf() based on the flags? For example: --------------------------------------------------- static char buf[128]; snprintf(buf, sizeof(buf), "%s%s%s%s%s%s%s%s", (flags & CHECKPOINT_IS_SHUTDOWN) ? " shutdown" : "", (flags & CHECKPOINT_END_OF_RECOVERY) ? " end-of-recovery" : "", (flags & CHECKPOINT_FAST) ? " fast" : "", (flags & CHECKPOINT_FORCE) ? " force" : "", (flags & CHECKPOINT_WAIT) ? " wait" : "", (flags & CHECKPOINT_CAUSE_XLOG) ? " wal" : "", (flags & CHECKPOINT_CAUSE_TIME) ? " time" : "", (flags & CHECKPOINT_FLUSH_UNLOGGED) ? " flush-unlogged" : ""); return buf; --------------------------------------------------- + * Format checkpoint reason flags consistently for log messages. + * The returned string is suitable for inclusion after + * "checkpoint starting:" or inside "checkpoint complete (...)". + */ +static const char * +CheckpointReasonString(int flags) These flags include more than just the reason a checkpoint was triggered, so using "reason" here seems misleading. Wouldn't just "flags" be a better term? Regards, -- Fujii Masao ^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: [PATCH] Expose checkpoint reason to completion log messages. @ 2026-01-14 10:11 Soumya S Murali <[email protected]> parent: Fujii Masao <[email protected]> 0 siblings, 1 reply; 13+ messages in thread From: Soumya S Murali @ 2026-01-14 10:11 UTC (permalink / raw) To: Fujii Masao <[email protected]>; +Cc: Michael Banck <[email protected]>; VASUKI M <[email protected]>; Andres Freund <[email protected]>; PostgreSQL Hackers <[email protected]>; Álvaro Herrera <[email protected]>; [email protected]; [email protected] Hi all, On Tue, Jan 13, 2026 at 6:40 PM Fujii Masao <[email protected]> wrote: > > On Tue, Jan 13, 2026 at 3:29 PM Soumya S Murali > <[email protected]> wrote: > > Thank you for the detailed review and suggestions. > > I have reworked the patch on a fresh tree, addressed the formatting > > and indentation issues, and aligned the helper implementation with > > PostgreSQL coding conventions. The updated patch has been pgindented > > and validated with make check and the full recovery TAP test suite. I > > am attaching the revised patch for further review. > > Thanks for updating the patch! > > With this patch, the checkpoint log messages look like this: > > LOG: checkpoint starting: fast force wait > LOG: checkpoint complete (fast force wait): ... > > The formatting of the checkpoint flags differs between the starting and > complete messages: the complete message wraps them in parentheses, > while the starting message does not. I think it would be better to log > the flags in a consistent way in both messages. For example: > > LOG: checkpoint starting: fast force wait > LOG: checkpoint complete: fast force wait: ... > > > - if ($node_primary->log_contains("checkpoint complete: ", $logstart)) > + if ($node_primary->log_contains(qr/checkpoint complete(?: > \([^)]*\))?:/, $logstart)) > > If we adopt a format like the above example, this test change would > no longer be needed. > > > +#define APPEND_REASON(str) \ > + do \ > + { \ > + if (!first) \ > > Wouldn't it be simpler to just build the string with snprintf() based on > the flags? For example: > > --------------------------------------------------- > static char buf[128]; > > snprintf(buf, sizeof(buf), "%s%s%s%s%s%s%s%s", > (flags & CHECKPOINT_IS_SHUTDOWN) ? " shutdown" : "", > (flags & CHECKPOINT_END_OF_RECOVERY) ? " end-of-recovery" : "", > (flags & CHECKPOINT_FAST) ? " fast" : "", > (flags & CHECKPOINT_FORCE) ? " force" : "", > (flags & CHECKPOINT_WAIT) ? " wait" : "", > (flags & CHECKPOINT_CAUSE_XLOG) ? " wal" : "", > (flags & CHECKPOINT_CAUSE_TIME) ? " time" : "", > (flags & CHECKPOINT_FLUSH_UNLOGGED) ? " flush-unlogged" : ""); > > return buf; > --------------------------------------------------- > > > + * Format checkpoint reason flags consistently for log messages. > + * The returned string is suitable for inclusion after > + * "checkpoint starting:" or inside "checkpoint complete (...)". > + */ > +static const char * > +CheckpointReasonString(int flags) > > These flags include more than just the reason a checkpoint was triggered, > so using "reason" here seems misleading. Wouldn't just "flags" be a better term? > Thank you for the detailed review and suggestions. Based on the feedback, I have updated the patch to make the checkpoint and restart point log formatting fully consistent between start and completion log messages. The completion messages now use the same format as the start messages without parentheses. I have also replaced the term "reason" with the term "flags". The patch has been rebuilt and validated with make check and the full recovery TAP test suite, and I have manually verified the resulting log output to confirm the expected formatting. I am attaching the updated patch for further review. Please let me know if any additional adjustments are needed. Regards, Soumya Attachments: [text/x-patch] 0001-Expose-checkpoint-reason-in-completion-log-messages.patch (6.0K, ../../CAMtXxw9T7W3pRQ9zm-WrEvLgyYFRi8DtBqP4K_j9jk4YK1FFSg@mail.gmail.com/2-0001-Expose-checkpoint-reason-in-completion-log-messages.patch) download | inline diff: From 6987dad63e07700bbde9e095516f880719b9c729 Mon Sep 17 00:00:00 2001 From: Soumya <[email protected]> Date: Wed, 14 Jan 2026 15:32:29 +0530 Subject: [PATCH] Expose checkpoint reason in completion log messages Signed-off-by: Soumya <[email protected]> --- src/backend/access/transam/xlog.c | 79 ++++++++++++++++------- src/test/recovery/t/019_replslot_limit.pl | 2 +- 2 files changed, 56 insertions(+), 25 deletions(-) diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 22d0a2e8c3..881e656f25 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -6714,6 +6714,50 @@ ShutdownXLOG(int code, Datum arg) } } +/* + * Format checkpoint reason flags consistently for log messages. + * The returned string is suitable for inclusion after + * "checkpoint starting:" or inside "checkpoint complete (...)". + */ +static const char * +CheckpointFlagsString(int flags) +{ + static char buf[128]; + bool first = true; + + buf[0] = '\0'; + +#define APPEND_REASON(str) \ + do \ + { \ + if (!first) \ + strcat(buf, " "); \ + strcat(buf, (str)); \ + first = false; \ + } while (0) + + if (flags & CHECKPOINT_IS_SHUTDOWN) + APPEND_REASON("shutdown"); + if (flags & CHECKPOINT_END_OF_RECOVERY) + APPEND_REASON("end-of-recovery"); + if (flags & CHECKPOINT_FAST) + APPEND_REASON("fast"); + if (flags & CHECKPOINT_FORCE) + APPEND_REASON("force"); + if (flags & CHECKPOINT_WAIT) + APPEND_REASON("wait"); + if (flags & CHECKPOINT_CAUSE_XLOG) + APPEND_REASON("wal"); + if (flags & CHECKPOINT_CAUSE_TIME) + APPEND_REASON("time"); + if (flags & CHECKPOINT_FLUSH_UNLOGGED) + APPEND_REASON("flush-unlogged"); + +#undef APPEND_REASON + + return buf; +} + /* * Log start of a checkpoint. */ @@ -6723,34 +6767,19 @@ LogCheckpointStart(int flags, bool restartpoint) if (restartpoint) ereport(LOG, /* translator: the placeholders show checkpoint options */ - (errmsg("restartpoint starting:%s%s%s%s%s%s%s%s", - (flags & CHECKPOINT_IS_SHUTDOWN) ? " shutdown" : "", - (flags & CHECKPOINT_END_OF_RECOVERY) ? " end-of-recovery" : "", - (flags & CHECKPOINT_FAST) ? " fast" : "", - (flags & CHECKPOINT_FORCE) ? " force" : "", - (flags & CHECKPOINT_WAIT) ? " wait" : "", - (flags & CHECKPOINT_CAUSE_XLOG) ? " wal" : "", - (flags & CHECKPOINT_CAUSE_TIME) ? " time" : "", - (flags & CHECKPOINT_FLUSH_UNLOGGED) ? " flush-unlogged" : ""))); + (errmsg("restartpoint starting: %s", + CheckpointFlagsString(flags)))); else ereport(LOG, - /* translator: the placeholders show checkpoint options */ - (errmsg("checkpoint starting:%s%s%s%s%s%s%s%s", - (flags & CHECKPOINT_IS_SHUTDOWN) ? " shutdown" : "", - (flags & CHECKPOINT_END_OF_RECOVERY) ? " end-of-recovery" : "", - (flags & CHECKPOINT_FAST) ? " fast" : "", - (flags & CHECKPOINT_FORCE) ? " force" : "", - (flags & CHECKPOINT_WAIT) ? " wait" : "", - (flags & CHECKPOINT_CAUSE_XLOG) ? " wal" : "", - (flags & CHECKPOINT_CAUSE_TIME) ? " time" : "", - (flags & CHECKPOINT_FLUSH_UNLOGGED) ? " flush-unlogged" : ""))); + (errmsg("checkpoint starting: %s", + CheckpointFlagsString(flags)))); } /* * Log end of a checkpoint. */ static void -LogCheckpointEnd(bool restartpoint) +LogCheckpointEnd(bool restartpoint, int flags) { long write_msecs, sync_msecs, @@ -6800,12 +6829,13 @@ LogCheckpointEnd(bool restartpoint) */ if (restartpoint) ereport(LOG, - (errmsg("restartpoint complete: wrote %d buffers (%.1f%%), " + (errmsg("restartpoint complete: %s: wrote %d buffers (%.1f%%), " "wrote %d SLRU buffers; %d WAL file(s) added, " "%d removed, %d recycled; write=%ld.%03d s, " "sync=%ld.%03d s, total=%ld.%03d s; sync files=%d, " "longest=%ld.%03d s, average=%ld.%03d s; distance=%d kB, " "estimate=%d kB; lsn=%X/%08X, redo lsn=%X/%08X", + CheckpointFlagsString(flags), CheckpointStats.ckpt_bufs_written, (double) CheckpointStats.ckpt_bufs_written * 100 / NBuffers, CheckpointStats.ckpt_slru_written, @@ -6824,12 +6854,13 @@ LogCheckpointEnd(bool restartpoint) LSN_FORMAT_ARGS(ControlFile->checkPointCopy.redo)))); else ereport(LOG, - (errmsg("checkpoint complete: wrote %d buffers (%.1f%%), " + (errmsg("checkpoint complete: %s: wrote %d buffers (%.1f%%), " "wrote %d SLRU buffers; %d WAL file(s) added, " "%d removed, %d recycled; write=%ld.%03d s, " "sync=%ld.%03d s, total=%ld.%03d s; sync files=%d, " "longest=%ld.%03d s, average=%ld.%03d s; distance=%d kB, " "estimate=%d kB; lsn=%X/%08X, redo lsn=%X/%08X", + CheckpointFlagsString(flags), CheckpointStats.ckpt_bufs_written, (double) CheckpointStats.ckpt_bufs_written * 100 / NBuffers, CheckpointStats.ckpt_slru_written, @@ -7418,7 +7449,7 @@ CreateCheckPoint(int flags) TruncateSUBTRANS(GetOldestTransactionIdConsideredRunning()); /* Real work is done; log and update stats. */ - LogCheckpointEnd(false); + LogCheckpointEnd(false, flags); /* Reset the process title */ update_checkpoint_display(flags, false, true); @@ -7886,7 +7917,7 @@ CreateRestartPoint(int flags) TruncateSUBTRANS(GetOldestTransactionIdConsideredRunning()); /* Real work is done; log and update stats. */ - LogCheckpointEnd(true); + LogCheckpointEnd(true, flags); /* Reset the process title */ update_checkpoint_display(flags, true, true); diff --git a/src/test/recovery/t/019_replslot_limit.pl b/src/test/recovery/t/019_replslot_limit.pl index 6468784b83..ad2088ef3a 100644 --- a/src/test/recovery/t/019_replslot_limit.pl +++ b/src/test/recovery/t/019_replslot_limit.pl @@ -203,7 +203,7 @@ is($result, "rep1|f|t|lost|", my $checkpoint_ended = 0; for (my $i = 0; $i < 10 * $PostgreSQL::Test::Utils::timeout_default; $i++) { - if ($node_primary->log_contains("checkpoint complete: ", $logstart)) + if ($node_primary->log_contains("checkpoint complete:", $logstart)) { $checkpoint_ended = 1; last; -- 2.34.1 ^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: [PATCH] Expose checkpoint reason to completion log messages. @ 2026-02-17 05:55 Fujii Masao <[email protected]> parent: Soumya S Murali <[email protected]> 0 siblings, 1 reply; 13+ messages in thread From: Fujii Masao @ 2026-02-17 05:55 UTC (permalink / raw) To: Soumya S Murali <[email protected]>; +Cc: Michael Banck <[email protected]>; VASUKI M <[email protected]>; Andres Freund <[email protected]>; PostgreSQL Hackers <[email protected]>; Álvaro Herrera <[email protected]>; [email protected]; [email protected] On Wed, Jan 14, 2026 at 7:11 PM Soumya S Murali <[email protected]> wrote: > Thank you for the detailed review and suggestions. > Based on the feedback, I have updated the patch to make the checkpoint > and restart point log formatting fully consistent between start and > completion log messages. The completion messages now use the same > format as the start messages without parentheses. I have also replaced > the term "reason" with the term "flags". The patch has been rebuilt > and validated with make check and the full recovery TAP test suite, > and I have manually verified the resulting log output to confirm the > expected formatting. I am attaching the updated patch for further > review. Please let me know if any additional adjustments are needed. Thanks for updating the patch! + if (flags & CHECKPOINT_IS_SHUTDOWN) + APPEND_REASON("shutdown"); + if (flags & CHECKPOINT_END_OF_RECOVERY) + APPEND_REASON("end-of-recovery"); I still think it would be simpler to construct the checkpoint flags string with snprintf() rather than introducing a new macro. Please see the attached patch. - if ($node_primary->log_contains("checkpoint complete: ", $logstart)) + if ($node_primary->log_contains("checkpoint complete:", $logstart)) I don't think this change is necessary. I've also updated the commit message in the attached patch. Regards, -- Fujii Masao Attachments: [application/octet-stream] v8-0001-Log-checkpoint-request-flags-in-checkpoint-comple.patch (5.9K, ../../CAHGQGwGLkdZ4FT73eczJXoVK_+DBxW7PAKiE8m4s48UcZJGKZQ@mail.gmail.com/2-v8-0001-Log-checkpoint-request-flags-in-checkpoint-comple.patch) download | inline diff: From d9f3cbf8502124550f4e5378f17d3a4b3c449c82 Mon Sep 17 00:00:00 2001 From: Fujii Masao <[email protected]> Date: Tue, 17 Feb 2026 12:27:34 +0900 Subject: [PATCH v8] Log checkpoint request flags in checkpoint completion messages. Checkpoint completion log messages include more detail than checkpoint start messages, but previously omitted the checkpoint request flags, which were only logged at checkpoint start. As a result, users had to correlate completion messages with earlier start messages to see the full context. This commit includes the checkpoint request flags in the checkpoint completion log message as well. This duplicates some information, but makes the completion message self-contained and easier to interpret. Author: Soumya S Murali <[email protected]> Reviewed-by: Michael Banck <[email protected]> Reviewed-by: Yuan Li <[email protected]> Reviewed-by: Fujii Masao <[email protected]> Discussion: https://postgr.es/m/CAMtXxw9tPwV=NBv5S9GZXMSKPeKv5f9hRhSjZ8__oLsoS5jcuA@mail.gmail.com --- src/backend/access/transam/xlog.c | 60 ++++++++++++++++++------------- 1 file changed, 35 insertions(+), 25 deletions(-) diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 13ec6225b85..13cce9b49f1 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -6768,6 +6768,28 @@ ShutdownXLOG(int code, Datum arg) } } +/* + * Format checkpoint request flags as a space-separated string for + * log messages. + */ +static const char * +CheckpointFlagsString(int flags) +{ + static char buf[128]; + + snprintf(buf, sizeof(buf), "%s%s%s%s%s%s%s%s", + (flags & CHECKPOINT_IS_SHUTDOWN) ? " shutdown" : "", + (flags & CHECKPOINT_END_OF_RECOVERY) ? " end-of-recovery" : "", + (flags & CHECKPOINT_FAST) ? " fast" : "", + (flags & CHECKPOINT_FORCE) ? " force" : "", + (flags & CHECKPOINT_WAIT) ? " wait" : "", + (flags & CHECKPOINT_CAUSE_XLOG) ? " wal" : "", + (flags & CHECKPOINT_CAUSE_TIME) ? " time" : "", + (flags & CHECKPOINT_FLUSH_UNLOGGED) ? " flush-unlogged" : ""); + + return buf; +} + /* * Log start of a checkpoint. */ @@ -6776,35 +6798,21 @@ LogCheckpointStart(int flags, bool restartpoint) { if (restartpoint) ereport(LOG, - /* translator: the placeholders show checkpoint options */ - (errmsg("restartpoint starting:%s%s%s%s%s%s%s%s", - (flags & CHECKPOINT_IS_SHUTDOWN) ? " shutdown" : "", - (flags & CHECKPOINT_END_OF_RECOVERY) ? " end-of-recovery" : "", - (flags & CHECKPOINT_FAST) ? " fast" : "", - (flags & CHECKPOINT_FORCE) ? " force" : "", - (flags & CHECKPOINT_WAIT) ? " wait" : "", - (flags & CHECKPOINT_CAUSE_XLOG) ? " wal" : "", - (flags & CHECKPOINT_CAUSE_TIME) ? " time" : "", - (flags & CHECKPOINT_FLUSH_UNLOGGED) ? " flush-unlogged" : ""))); + /* translator: the placeholder shows checkpoint options */ + (errmsg("restartpoint starting:%s", + CheckpointFlagsString(flags)))); else ereport(LOG, - /* translator: the placeholders show checkpoint options */ - (errmsg("checkpoint starting:%s%s%s%s%s%s%s%s", - (flags & CHECKPOINT_IS_SHUTDOWN) ? " shutdown" : "", - (flags & CHECKPOINT_END_OF_RECOVERY) ? " end-of-recovery" : "", - (flags & CHECKPOINT_FAST) ? " fast" : "", - (flags & CHECKPOINT_FORCE) ? " force" : "", - (flags & CHECKPOINT_WAIT) ? " wait" : "", - (flags & CHECKPOINT_CAUSE_XLOG) ? " wal" : "", - (flags & CHECKPOINT_CAUSE_TIME) ? " time" : "", - (flags & CHECKPOINT_FLUSH_UNLOGGED) ? " flush-unlogged" : ""))); + /* translator: the placeholder shows checkpoint options */ + (errmsg("checkpoint starting:%s", + CheckpointFlagsString(flags)))); } /* * Log end of a checkpoint. */ static void -LogCheckpointEnd(bool restartpoint) +LogCheckpointEnd(bool restartpoint, int flags) { long write_msecs, sync_msecs, @@ -6854,12 +6862,13 @@ LogCheckpointEnd(bool restartpoint) */ if (restartpoint) ereport(LOG, - (errmsg("restartpoint complete: wrote %d buffers (%.1f%%), " + (errmsg("restartpoint complete:%s: wrote %d buffers (%.1f%%), " "wrote %d SLRU buffers; %d WAL file(s) added, " "%d removed, %d recycled; write=%ld.%03d s, " "sync=%ld.%03d s, total=%ld.%03d s; sync files=%d, " "longest=%ld.%03d s, average=%ld.%03d s; distance=%d kB, " "estimate=%d kB; lsn=%X/%08X, redo lsn=%X/%08X", + CheckpointFlagsString(flags), CheckpointStats.ckpt_bufs_written, (double) CheckpointStats.ckpt_bufs_written * 100 / NBuffers, CheckpointStats.ckpt_slru_written, @@ -6878,12 +6887,13 @@ LogCheckpointEnd(bool restartpoint) LSN_FORMAT_ARGS(ControlFile->checkPointCopy.redo)))); else ereport(LOG, - (errmsg("checkpoint complete: wrote %d buffers (%.1f%%), " + (errmsg("checkpoint complete:%s: wrote %d buffers (%.1f%%), " "wrote %d SLRU buffers; %d WAL file(s) added, " "%d removed, %d recycled; write=%ld.%03d s, " "sync=%ld.%03d s, total=%ld.%03d s; sync files=%d, " "longest=%ld.%03d s, average=%ld.%03d s; distance=%d kB, " "estimate=%d kB; lsn=%X/%08X, redo lsn=%X/%08X", + CheckpointFlagsString(flags), CheckpointStats.ckpt_bufs_written, (double) CheckpointStats.ckpt_bufs_written * 100 / NBuffers, CheckpointStats.ckpt_slru_written, @@ -7480,7 +7490,7 @@ CreateCheckPoint(int flags) TruncateSUBTRANS(GetOldestTransactionIdConsideredRunning()); /* Real work is done; log and update stats. */ - LogCheckpointEnd(false); + LogCheckpointEnd(false, flags); /* Reset the process title */ update_checkpoint_display(flags, false, true); @@ -7951,7 +7961,7 @@ CreateRestartPoint(int flags) TruncateSUBTRANS(GetOldestTransactionIdConsideredRunning()); /* Real work is done; log and update stats. */ - LogCheckpointEnd(true); + LogCheckpointEnd(true, flags); /* Reset the process title */ update_checkpoint_display(flags, true, true); -- 2.51.2 ^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: [PATCH] Expose checkpoint reason to completion log messages. @ 2026-02-17 10:47 Soumya S Murali <[email protected]> parent: Fujii Masao <[email protected]> 0 siblings, 1 reply; 13+ messages in thread From: Soumya S Murali @ 2026-02-17 10:47 UTC (permalink / raw) To: Fujii Masao <[email protected]>; +Cc: Michael Banck <[email protected]>; VASUKI M <[email protected]>; Andres Freund <[email protected]>; PostgreSQL Hackers <[email protected]>; Álvaro Herrera <[email protected]>; [email protected]; [email protected] Hi all, On Tue, Feb 17, 2026 at 11:25 AM Fujii Masao <[email protected]> wrote: > > On Wed, Jan 14, 2026 at 7:11 PM Soumya S Murali > <[email protected]> wrote: > > Thank you for the detailed review and suggestions. > > Based on the feedback, I have updated the patch to make the checkpoint > > and restart point log formatting fully consistent between start and > > completion log messages. The completion messages now use the same > > format as the start messages without parentheses. I have also replaced > > the term "reason" with the term "flags". The patch has been rebuilt > > and validated with make check and the full recovery TAP test suite, > > and I have manually verified the resulting log output to confirm the > > expected formatting. I am attaching the updated patch for further > > review. Please let me know if any additional adjustments are needed. > > Thanks for updating the patch! > > + if (flags & CHECKPOINT_IS_SHUTDOWN) > + APPEND_REASON("shutdown"); > + if (flags & CHECKPOINT_END_OF_RECOVERY) > + APPEND_REASON("end-of-recovery"); > > I still think it would be simpler to construct the checkpoint flags string > with snprintf() rather than introducing a new macro. Please see > the attached patch. > > - if ($node_primary->log_contains("checkpoint complete: ", $logstart)) > + if ($node_primary->log_contains("checkpoint complete:", $logstart)) > > I don't think this change is necessary. > > I've also updated the commit message in the attached patch. Thank you very much for refining the implementation and simplifying the flag construction logic using snprintf(). I have reviewed the committed changes and verified that the formatting and behavior are consistent and correct. The updated approach is clear and aligns well with PostgreSQL coding style. I also appreciate the improvements made to the commit message as it clearly explains the motivation and makes the change easy to understand in context. I sincerely appreciate your guidance throughout this review process. Regards, Soumya ^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: [PATCH] Expose checkpoint reason to completion log messages. @ 2026-02-19 14:57 Fujii Masao <[email protected]> parent: Soumya S Murali <[email protected]> 0 siblings, 1 reply; 13+ messages in thread From: Fujii Masao @ 2026-02-19 14:57 UTC (permalink / raw) To: Soumya S Murali <[email protected]>; +Cc: Michael Banck <[email protected]>; VASUKI M <[email protected]>; Andres Freund <[email protected]>; PostgreSQL Hackers <[email protected]>; Álvaro Herrera <[email protected]>; [email protected]; [email protected] On Tue, Feb 17, 2026 at 7:47 PM Soumya S Murali <[email protected]> wrote: > Thank you very much for refining the implementation and simplifying > the flag construction logic using snprintf(). I have reviewed the > committed changes and verified that the formatting and behavior are > consistent and correct. The updated approach is clear and aligns well > with PostgreSQL coding style. I also appreciate the improvements made > to the commit message as it clearly explains the motivation and makes > the change easy to understand in context. I've pushed the patch. Thanks! Regards, -- Fujii Masao ^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: [PATCH] Expose checkpoint reason to completion log messages. @ 2026-02-20 05:25 Soumya S Murali <[email protected]> parent: Fujii Masao <[email protected]> 0 siblings, 0 replies; 13+ messages in thread From: Soumya S Murali @ 2026-02-20 05:25 UTC (permalink / raw) To: Fujii Masao <[email protected]>; +Cc: Michael Banck <[email protected]>; VASUKI M <[email protected]>; Andres Freund <[email protected]>; PostgreSQL Hackers <[email protected]>; Álvaro Herrera <[email protected]>; [email protected]; [email protected] Hi all, On Thu, Feb 19, 2026 at 8:27 PM Fujii Masao <[email protected]> wrote: > > On Tue, Feb 17, 2026 at 7:47 PM Soumya S Murali > <[email protected]> wrote: > > Thank you very much for refining the implementation and simplifying > > the flag construction logic using snprintf(). I have reviewed the > > committed changes and verified that the formatting and behavior are > > consistent and correct. The updated approach is clear and aligns well > > with PostgreSQL coding style. I also appreciate the improvements made > > to the commit message as it clearly explains the motivation and makes > > the change easy to understand in context. > > I've pushed the patch. Thanks! > Thank you so much for pushing the patch. I truly appreciate your guidance and the detailed review throughout this process. It has been a valuable learning experience. Thank you. Regards, Soumya ^ permalink raw reply [nested|flat] 13+ messages in thread
end of thread, other threads:[~2026-02-20 05:25 UTC | newest] Thread overview: 13+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2023-05-31 11:46 [PATCH v28 08/11] Add aggregates support in IVM Yugo Nagata <[email protected]> 2026-01-08 06:28 Re: [PATCH] Expose checkpoint reason to completion log messages. Soumya S Murali <[email protected]> 2026-01-08 13:31 ` 回复: [PATCH] Expose checkpoint reason to completion log messages. li carol <[email protected]> 2026-01-08 17:36 ` Re: [PATCH] Expose checkpoint reason to completion log messages. Michael Banck <[email protected]> 2026-01-12 07:23 ` Re: [PATCH] Expose checkpoint reason to completion log messages. Soumya S Murali <[email protected]> 2026-01-12 10:26 ` Re: [PATCH] Expose checkpoint reason to completion log messages. Michael Banck <[email protected]> 2026-01-13 06:29 ` Re: [PATCH] Expose checkpoint reason to completion log messages. Soumya S Murali <[email protected]> 2026-01-13 13:09 ` Re: [PATCH] Expose checkpoint reason to completion log messages. Fujii Masao <[email protected]> 2026-01-14 10:11 ` Re: [PATCH] Expose checkpoint reason to completion log messages. Soumya S Murali <[email protected]> 2026-02-17 05:55 ` Re: [PATCH] Expose checkpoint reason to completion log messages. Fujii Masao <[email protected]> 2026-02-17 10:47 ` Re: [PATCH] Expose checkpoint reason to completion log messages. Soumya S Murali <[email protected]> 2026-02-19 14:57 ` Re: [PATCH] Expose checkpoint reason to completion log messages. Fujii Masao <[email protected]> 2026-02-20 05:25 ` Re: [PATCH] Expose checkpoint reason to completion log messages. Soumya S Murali <[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