public inbox for [email protected]help / color / mirror / Atom feed
[PATCH] Clean up pg_checksums.sgml 4+ messages / 4 participants [nested] [flat]
* [PATCH] Clean up pg_checksums.sgml @ 2019-03-29 00:20 Justin Pryzby <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: Justin Pryzby @ 2019-03-29 00:20 UTC (permalink / raw) --- doc/src/sgml/ref/pg_checksums.sgml | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/doc/src/sgml/ref/pg_checksums.sgml b/doc/src/sgml/ref/pg_checksums.sgml index c60695d..9433782 100644 --- a/doc/src/sgml/ref/pg_checksums.sgml +++ b/doc/src/sgml/ref/pg_checksums.sgml @@ -39,15 +39,16 @@ PostgreSQL documentation <application>pg_checksums</application> verifies, enables or disables data checksums in a <productname>PostgreSQL</productname> cluster. The server must be shut down cleanly before running - <application>pg_checksums</application>. The exit status is zero if there - are no checksum errors when checking them, and nonzero if at least one - checksum failure is detected. If enabling or disabling checksums, the + <application>pg_checksums</application>. When verifying checksums, the exit + status is zero if there are no checksum errors, and nonzero if at least one + checksum failure is detected. When enabling or disabling checksums, the exit status is nonzero if the operation failed. </para> <para> - While verifying or enabling checksums needs to scan or write every file in - the cluster, disabling checksums will only update the file + When verifying checksums, every file in the cluster is scanned; + When enabling checksums, every file in the cluster is also rewritten. + Disabling checksums only updates the file <filename>pg_control</filename>. </para> </refsect1> @@ -195,11 +196,10 @@ PostgreSQL documentation safe. </para> <para> - If <application>pg_checksums</application> is aborted or killed while - enabling or disabling checksums, the cluster will keep the same - configuration for data checksums as before the operation attempted. - <application>pg_checksums</application> can be restarted to - attempt again the same operation. + If <application>pg_checksums</application> is aborted or killed + while enabling or disabling checksums, the cluster's checksum state + will be unchanged, and <application>pg_checksums</application> would need to + be rerun and start its operation from scratch. </para> </refsect1> </refentry> -- 2.1.4 --wq9mPyueHGvFACwf-- ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: LWLock deadlock in brinRevmapDesummarizeRange @ 2023-02-22 14:41 Alvaro Herrera <[email protected]> 0 siblings, 1 reply; 4+ messages in thread From: Alvaro Herrera @ 2023-02-22 14:41 UTC (permalink / raw) To: Tomas Vondra <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]> On 2023-Feb-22, Tomas Vondra wrote: > > ... and in there, I wrote that we would first write the brin tuple in > > the regular page, unlock that, and then lock the revmap for the update, > > without holding lock on the data page. I don't remember why we do it > > differently now, but maybe the fix is just to release the regular page > > lock before locking the revmap page? One very important change is that > > in previous versions the revmap used a separate fork, and we had to > > introduce an "evacuation protocol" when we integrated the revmap into > > the main fork, which may have changed the locking considerations. > > What would happen if two processes built the summary concurrently? How > would they find the other tuple, so that we don't end up with two BRIN > tuples for the same range? Well, the revmap can only keep track of one tuple per range; if two processes build summary tuples, and each tries to insert its tuple in a regular page, that part may succeed; but then only one of them is going to successfully register the summary tuple in the revmap: when the other goes to do the same, it would find that a CTID is already present. ... Looking at the code (brinSetHeapBlockItemptr), I think what happens here is that the second process would overwrite the TID with its own. Not sure if it would work to see whether the item is empty and bail out if it's not. But in any case, it seems to me that the update of the regular page is pretty much independent of the update of the revmap. > > Another point: to desummarize a range, just unlinking the entry from > > revmap should suffice, from the POV of other index scanners. Maybe we > > can simplify the whole procedure to: lock revmap, remove entry, remember > > page number, unlock revmap; lock regular page, delete entry, unlock. > > Then there are no two locks held at the same time during desummarize. > > Perhaps, as long as it doesn't confuse anything else. Well, I don't have the details fresh in mind, but I think it shouldn't, because the only way to reach a regular tuple is coming from the revmap; and we reuse "items" (lines) in a regular page only when they are empty (so vacuuming should also be OK). > > This comes from v16: > > I don't follow - what do you mean by v16? I don't see anything like that > anywhere in the repository. I meant the minmax-proposal file in patch v16, the one that I linked to. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "You're _really_ hosed if the person doing the hiring doesn't understand relational systems: you end up with a whole raft of programmers, none of whom has had a Date with the clue stick." (Andrew Sullivan) ^ permalink raw reply [nested|flat] 4+ messages in thread
* [PATCH v38 08/11] Add aggregates support in IVM @ 2023-05-31 11:46 Yugo Nagata <[email protected]> 0 siblings, 0 replies; 4+ 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 | 275 ++++++++++++++++++-- src/backend/commands/matview.c | 433 ++++++++++++++++++++++++++++++-- src/include/commands/createas.h | 1 + 3 files changed, 672 insertions(+), 37 deletions(-) diff --git a/src/backend/commands/createas.c b/src/backend/commands/createa= s.c index cd8db0059f9..35e124694ab 100644 --- a/src/backend/commands/createas.c +++ b/src/backend/commands/createas.c @@ -56,12 +56,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/fmgroids.h" #include "utils/lsyscache.h" +#include "utils/regproc.h" +#include "utils/rel.h" #include "utils/rls.h" #include "utils/snapmgr.h" +#include "utils/syscache.h" =20 typedef struct { @@ -75,6 +82,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 +101,9 @@ static void CreateIvmTriggersOnBaseTablesRecurse(Query *= qry, Node *node, Oid mat List **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 @@ -424,6 +437,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,19 +448,61 @@ rewriteQueryForIMMV(Query *query, List *colNames) ParseState *pstate =3D make_parsestate(NULL); FuncCall *fn; =20 + /* + * Check the length of column name list not to override names of + * additional columns + */ + if (list_length(colNames) > list_length(query->targetList)) + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("too many column names were specified"))); + 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 @@ -463,6 +519,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 * @@ -946,11 +1087,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; @@ -979,6 +1122,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), @@ -1046,6 +1193,8 @@ check_ivm_restriction_walker(Node *node, void *contex= t) format_type_be(atttype), "btree"))); } =20 + context->has_agg |=3D qry->hasAggs; + /* restrictions for rtable */ foreach(lc, qry->rtable) { @@ -1094,7 +1243,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; } @@ -1105,8 +1254,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: @@ -1118,14 +1271,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; @@ -1133,6 +1308,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 * @@ -1190,7 +1405,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) @@ -1248,7 +1485,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 a2746ca9265..710224aa994 100644 --- a/src/backend/commands/matview.c +++ b/src/backend/commands/matview.c @@ -26,6 +26,7 @@ #include "catalog/pg_depend.h" #include "catalog/pg_trigger.h" #include "catalog/pg_opclass.h" +#include "commands/defrem.h" #include "commands/matview.h" #include "commands/repack.h" #include "commands/tablecmds.h" @@ -36,6 +37,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" @@ -152,6 +154,13 @@ IvmShmemRequest(void *arg) =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" @@ -183,7 +192,7 @@ static RangeTblEntry *get_prestate_rte(RangeTblEntry *r= te, MV_TriggerTable *tabl QueryEnvironment *queryEnv, Oid matviewid); static RangeTblEntry *makeDeltaTable(RangeTblEntry *rte, MV_TriggerTable *= 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, @@ -194,14 +203,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); @@ -1607,11 +1629,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 i= t + * 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, "",= false); + 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, InvalidSubTransactionId); @@ -1652,8 +1707,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) @@ -1704,7 +1759,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 */ @@ -2165,17 +2220,34 @@ makeDeltaTable(RangeTblEntry *rte, MV_TriggerTable = *table, } =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; @@ -2248,6 +2320,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 * @@ -2261,6 +2335,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; @@ -2283,6 +2360,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++) { @@ -2299,13 +2385,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 thes= e + * 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. */ @@ -2336,7 +2470,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 @@ -2362,7 +2497,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); } @@ -2377,6 +2512,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 * @@ -2384,13 +2763,20 @@ apply_delta(Oid matviewOid, Tuplestorestate *old_tu= plestores, Tuplestorestate *n * which contains tuples to be deleted from to a materialized view given b= y * 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 key= s + * 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); @@ -2400,22 +2786,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) @@ -2479,10 +2869,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 key= s + * 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; @@ -2513,6 +2908,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 */ @@ -2520,6 +2916,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 bfd0249b10d..313b129f7e8 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.43.0 --Multipart=_Wed__1_Jul_2026_00_04_01_+0900_OVSy2WWK_9aByzDJ Content-Type: text/x-diff; name="v38-0007-Add-DISTINCT-support-for-IVM.patch" Content-Disposition: attachment; filename="v38-0007-Add-DISTINCT-support-for-IVM.patch" Content-Transfer-Encoding: 7bit ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: LWLock deadlock in brinRevmapDesummarizeRange @ 2025-04-02 19:50 Arseniy Mukhin <[email protected]> parent: Alvaro Herrera <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: Arseniy Mukhin @ 2025-04-02 19:50 UTC (permalink / raw) To: Alvaro Herrera <[email protected]>; Tomas Vondra <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]> Hello Tomas and Alvaro, Came across this old thread while searching for information about brin, and decided to try to fix it. On 22.02.2023 17:41, Alvaro Herrera wrote: > On 2023-Feb-22, Tomas Vondra wrote: > >>> ... and in there, I wrote that we would first write the brin tuple in >>> the regular page, unlock that, and then lock the revmap for the update, >>> without holding lock on the data page. I don't remember why we do it >>> differently now, but maybe the fix is just to release the regular page >>> lock before locking the revmap page? One very important change is that >>> in previous versions the revmap used a separate fork, and we had to >>> introduce an "evacuation protocol" when we integrated the revmap into >>> the main fork, which may have changed the locking considerations. >> >> What would happen if two processes built the summary concurrently? How >> would they find the other tuple, so that we don't end up with two BRIN >> tuples for the same range? > > Well, the revmap can only keep track of one tuple per range; if two > processes build summary tuples, and each tries to insert its tuple in a > regular page, that part may succeed; but then only one of them is going > to successfully register the summary tuple in the revmap: when the other > goes to do the same, it would find that a CTID is already present. > > ... Looking at the code (brinSetHeapBlockItemptr), I think what happens > here is that the second process would overwrite the TID with its own. > Not sure if it would work to see whether the item is empty and bail out > if it's not. > AFAIS it's impossible to have two processes summarizing or desummarizing ranges simultaneously, because you need ShareUpdateExclusiveLock to do it. Usual inserts / updates can't lead to summarization, they affects only those ranges that are summarized already. >>> Another point: to desummarize a range, just unlinking the entry from >>> revmap should suffice, from the POV of other index scanners. Maybe we >>> can simplify the whole procedure to: lock revmap, remove entry, remember >>> page number, unlock revmap; lock regular page, delete entry, unlock. >>> Then there are no two locks held at the same time during desummarize. >> >> Perhaps, as long as it doesn't confuse anything else. > > Well, I don't have the details fresh in mind, but I think it shouldn't, > because the only way to reach a regular tuple is coming from the revmap; > and we reuse "items" (lines) in a regular page only when they are > empty (so vacuuming should also be OK). > I think it would work, but there's one thing to handle in this case: You may have a concurrent update that moved the index tuple to another reg page and wants to update the revmap page with a new pointer. This may happen after the revmap entry is deleted, so you can have kind of lost update. But this seems to be easy to detect: when you try to delete the index tuple, you'll see an unused item. This means that a concurrent update moved index tuple and updated revmap page and you need to retry the desummarization. Also if you don't hold locks on both revmap page and regular pages, you will have to split wal record and rework recovery for desummarization. And probably in case of crash between revmap update and regular page update you can have dangling index tuple, so it seems that having one wal record for update simplifies recovery. I think there is a straightforward way to implement desummarization and fix the deadlock. Desummarization is actually very similar to usual update, so we can just use update flow (like brininsert or second part of summarize_range where you need to merge summary with placeholder tuple): - lock revmap page - save pointer to the index tuple - unlock revmap page - lock regular page - check if pointer is still valid (blkno is as expected, NORMAL tuple etc.) if we see that pointer is not valid anymore -> unlock reg page + retry - lock revmap page - update reg page and revmap page First five steps have been implemented already in brinGetTupleForHeapBlock, so we can use it (like every update actually do, if I see correctly). Please find the attached patch implementing the above idea. I tried Tomas's stress tests with applied patch and didn't have deadlocks. Also there is a another way how issue can be reproduced: apply deadlock-sleep.patch (it just adds 15 sec sleep in brin_doupdate and few logs) execute deadlock.sql execute deadlock2.sql from another client (there are 15 seconds to do it while first process is sleeping). Best regards, Arseniy Mukhin Attachments: [text/x-patch] 0001-brin-desummarization-deadlock.patch (5.7K, ../../[email protected]/2-0001-brin-desummarization-deadlock.patch) download | inline diff: From 31a8a558b845cda6b1ec1d18829e0f7ff0ec350d Mon Sep 17 00:00:00 2001 From: Arseniy Mukhin <[email protected]> Date: Wed, 2 Apr 2025 21:55:24 +0300 Subject: [PATCH] brin desummarization deadlock --- src/backend/access/brin/brin.c | 8 +-- src/backend/access/brin/brin_revmap.c | 71 ++++++++++++--------------- src/include/access/brin_revmap.h | 2 +- 3 files changed, 33 insertions(+), 48 deletions(-) diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c index 01e1db7f856..c52677534ed 100644 --- a/src/backend/access/brin/brin.c +++ b/src/backend/access/brin/brin.c @@ -1496,7 +1496,6 @@ brin_desummarize_range(PG_FUNCTION_ARGS) Oid heapoid; Relation heapRel; Relation indexRel; - bool done; if (RecoveryInProgress()) ereport(ERROR, @@ -1555,12 +1554,7 @@ brin_desummarize_range(PG_FUNCTION_ARGS) /* see gin_clean_pending_list() */ if (indexRel->rd_index->indisvalid) { - /* the revmap does the hard work */ - do - { - done = brinRevmapDesummarizeRange(indexRel, heapBlk); - } - while (!done); + brinRevmapDesummarizeRange(indexRel, heapBlk); } else ereport(DEBUG1, diff --git a/src/backend/access/brin/brin_revmap.c b/src/backend/access/brin/brin_revmap.c index 4e380ecc710..d3ef03d34ae 100644 --- a/src/backend/access/brin/brin_revmap.c +++ b/src/backend/access/brin/brin_revmap.c @@ -316,10 +316,8 @@ brinGetTupleForHeapBlock(BrinRevmap *revmap, BlockNumber heapBlk, * Delete an index tuple, marking a page range as unsummarized. * * Index must be locked in ShareUpdateExclusiveLock mode. - * - * Return false if caller should retry. */ -bool +void brinRevmapDesummarizeRange(Relation idxrel, BlockNumber heapBlk) { BrinRevmap *revmap; @@ -327,26 +325,37 @@ brinRevmapDesummarizeRange(Relation idxrel, BlockNumber heapBlk) RevmapContents *contents; ItemPointerData *iptr; ItemPointerData invalidIptr; - BlockNumber revmapBlk; Buffer revmapBuf; - Buffer regBuf; + Buffer regBuf = InvalidBuffer; Page revmapPg; Page regPg; OffsetNumber revmapOffset; OffsetNumber regOffset; - ItemId lp; + BrinTuple *btup; + Size size; revmap = brinRevmapInitialize(idxrel, &pagesPerRange); - revmapBlk = revmap_get_blkno(revmap, heapBlk); - if (!BlockNumberIsValid(revmapBlk)) + /* Try to get index tuple for the target block number */ + btup = brinGetTupleForHeapBlock(revmap, heapBlk, ®Buf, ®Offset, &size, BUFFER_LOCK_EXCLUSIVE); + + /* If range is not summarized, release resources and we are done */ + if (!btup) { - /* revmap page doesn't exist: range not summarized, we're done */ + if (BufferIsValid(regBuf)) + { + ReleaseBuffer(regBuf); + } brinRevmapTerminate(revmap); - return true; + + return; } - /* Lock the revmap page, obtain the index tuple pointer from it */ + /* + * We know that range is summarized (or it's a dangling placeholder), and + * we have the regular page with the index tuple locked exclusively. Now + * lock the revmap page with pointer to our index tuple + */ revmapBuf = brinLockRevmapPageForUpdate(revmap, heapBlk); revmapPg = BufferGetPage(revmapBuf); revmapOffset = HEAPBLK_TO_REVMAP_INDEX(revmap->rm_pagesPerRange, heapBlk); @@ -355,38 +364,22 @@ brinRevmapDesummarizeRange(Relation idxrel, BlockNumber heapBlk) iptr = contents->rm_tids; iptr += revmapOffset; - if (!ItemPointerIsValid(iptr)) - { - /* no index tuple: range not summarized, we're done */ - LockBuffer(revmapBuf, BUFFER_LOCK_UNLOCK); - brinRevmapTerminate(revmap); - return true; - } - - regBuf = ReadBuffer(idxrel, ItemPointerGetBlockNumber(iptr)); - LockBuffer(regBuf, BUFFER_LOCK_EXCLUSIVE); - regPg = BufferGetPage(regBuf); - - /* if this is no longer a regular page, tell caller to start over */ - if (!BRIN_IS_REGULAR_PAGE(regPg)) + /* + * No concurrent updates for the range are possible as any update should + * lock regular page first, and we already hold the lock. We know that + * index tuple for the range exists, so revmap item pointer couldn't be + * invalid and should point to index tuple btup. + */ + if (!ItemPointerIsValid(iptr) || + ItemPointerGetBlockNumber(iptr) != BufferGetBlockNumber(regBuf) || + ItemPointerGetOffsetNumber(iptr) != regOffset) { - LockBuffer(revmapBuf, BUFFER_LOCK_UNLOCK); - LockBuffer(regBuf, BUFFER_LOCK_UNLOCK); - brinRevmapTerminate(revmap); - return false; - } - - regOffset = ItemPointerGetOffsetNumber(iptr); - if (regOffset > PageGetMaxOffsetNumber(regPg)) ereport(ERROR, (errcode(ERRCODE_INDEX_CORRUPTED), errmsg("corrupted BRIN index: inconsistent range map"))); + } - lp = PageGetItemId(regPg, regOffset); - if (!ItemIdIsUsed(lp)) - ereport(ERROR, - (errcode(ERRCODE_INDEX_CORRUPTED), - errmsg("corrupted BRIN index: inconsistent range map"))); + regPg = BufferGetPage(regBuf); /* * Placeholder tuples only appear during unfinished summarization, and we @@ -429,8 +422,6 @@ brinRevmapDesummarizeRange(Relation idxrel, BlockNumber heapBlk) UnlockReleaseBuffer(regBuf); LockBuffer(revmapBuf, BUFFER_LOCK_UNLOCK); brinRevmapTerminate(revmap); - - return true; } /* diff --git a/src/include/access/brin_revmap.h b/src/include/access/brin_revmap.h index b88473c9860..322109e903d 100644 --- a/src/include/access/brin_revmap.h +++ b/src/include/access/brin_revmap.h @@ -36,6 +36,6 @@ extern void brinSetHeapBlockItemptr(Buffer buf, BlockNumber pagesPerRange, extern BrinTuple *brinGetTupleForHeapBlock(BrinRevmap *revmap, BlockNumber heapBlk, Buffer *buf, OffsetNumber *off, Size *size, int mode); -extern bool brinRevmapDesummarizeRange(Relation idxrel, BlockNumber heapBlk); +extern void brinRevmapDesummarizeRange(Relation idxrel, BlockNumber heapBlk); #endif /* BRIN_REVMAP_H */ -- 2.43.0 [application/sql] deadlock.sql (1.1K, ../../[email protected]/3-deadlock.sql) download [application/sql] deadlock2.sql (124B, ../../[email protected]/4-deadlock2.sql) download [text/x-patch] deadlock-sleep.patch (649B, ../../[email protected]/5-deadlock-sleep.patch) download | inline diff: diff --git a/src/backend/access/brin/brin_pageops.c b/src/backend/access/brin/brin_pageops.c index 6d8dd1512d6..9c3ad1f0bab 100644 --- a/src/backend/access/brin/brin_pageops.c +++ b/src/backend/access/brin/brin_pageops.c @@ -237,8 +237,11 @@ brin_doupdate(Relation idxrel, BlockNumber pagesPerRange, OffsetNumber newoff; Size freespace = 0; + elog(DEBUG1, "sleep while update"); + sleep(15); + elog(DEBUG1, "Try to revmap for update reg page - update"); revmapbuf = brinLockRevmapPageForUpdate(revmap, heapBlk); - + elog(DEBUG1, "Try to revmap for update reg page - update"); START_CRIT_SECTION(); /* ^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2025-04-02 19:50 UTC | newest] Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2019-03-29 00:20 [PATCH] Clean up pg_checksums.sgml Justin Pryzby <[email protected]> 2023-02-22 14:41 Re: LWLock deadlock in brinRevmapDesummarizeRange Alvaro Herrera <[email protected]> 2025-04-02 19:50 ` Re: LWLock deadlock in brinRevmapDesummarizeRange Arseniy Mukhin <[email protected]> 2023-05-31 11:46 [PATCH v38 08/11] Add aggregates support in IVM Yugo Nagata <[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