public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v22 1/5] Change REINDEX/CLUSTER to accept an option list.. 5+ messages / 5 participants [nested] [flat]
* [PATCH v22 1/5] Change REINDEX/CLUSTER to accept an option list.. @ 2020-03-27 22:50 Justin Pryzby <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: Justin Pryzby @ 2020-03-27 22:50 UTC (permalink / raw) ..like EXPLAIN (..), VACUUM (..), and ANALYZE (..). Change docs in the style of VACUUM. See also: 52dcfda48778d16683c64ca4372299a099a15b96 --- doc/src/sgml/ref/cluster.sgml | 29 ++++++++++++++++++--- doc/src/sgml/ref/reindex.sgml | 43 +++++++++++++++++++++----------- src/backend/commands/cluster.c | 23 ++++++++++++++++- src/backend/commands/indexcmds.c | 41 ++++++++++++++++-------------- src/backend/nodes/copyfuncs.c | 2 ++ src/backend/nodes/equalfuncs.c | 2 ++ src/backend/parser/gram.y | 35 +++++++++++++++----------- src/backend/tcop/utility.c | 36 +++++++++++++++++++++++--- src/bin/psql/tab-complete.c | 23 +++++++++++++---- src/include/commands/cluster.h | 3 ++- src/include/commands/defrem.h | 7 +++--- src/include/nodes/parsenodes.h | 2 ++ 12 files changed, 180 insertions(+), 66 deletions(-) diff --git a/doc/src/sgml/ref/cluster.sgml b/doc/src/sgml/ref/cluster.sgml index 4da60d8d56..a6df8a3d81 100644 --- a/doc/src/sgml/ref/cluster.sgml +++ b/doc/src/sgml/ref/cluster.sgml @@ -21,8 +21,13 @@ PostgreSQL documentation <refsynopsisdiv> <synopsis> -CLUSTER [VERBOSE] <replaceable class="parameter">table_name</replaceable> [ USING <replaceable class="parameter">index_name</replaceable> ] -CLUSTER [VERBOSE] +CLUSTER [VERBOSE] [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] <replaceable class="parameter">table_name</replaceable> [ USING <replaceable class="parameter">index_name</replaceable> ] +CLUSTER [VERBOSE] [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] + +<phrase>where <replaceable class="parameter">option</replaceable> can be one of:</phrase> + + VERBOSE [ <replaceable class="parameter">boolean</replaceable> ] + </synopsis> </refsynopsisdiv> @@ -81,6 +86,16 @@ CLUSTER [VERBOSE] <title>Parameters</title> <variablelist> + <varlistentry> + <term><literal>VERBOSE</literal></term> + <listitem> + <para> + Prints a progress report as each table is clustered. +<!-- When specified within parenthesis, <literal>VERBOSE</literal> may be followed by a boolean ...--> + </para> + </listitem> + </varlistentry> + <varlistentry> <term><replaceable class="parameter">table_name</replaceable></term> <listitem> @@ -100,13 +115,19 @@ CLUSTER [VERBOSE] </varlistentry> <varlistentry> - <term><literal>VERBOSE</literal></term> + <term><replaceable class="parameter">boolean</replaceable></term> <listitem> <para> - Prints a progress report as each table is clustered. + Specifies whether the selected option should be turned on or off. + You can write <literal>TRUE</literal>, <literal>ON</literal>, or + <literal>1</literal> to enable the option, and <literal>FALSE</literal>, + <literal>OFF</literal>, or <literal>0</literal> to disable it. The + <replaceable class="parameter">boolean</replaceable> value can also + be omitted, in which case <literal>TRUE</literal> is assumed. </para> </listitem> </varlistentry> + </variablelist> </refsect1> diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml index aac5d5be23..ff369a7313 100644 --- a/doc/src/sgml/ref/reindex.sgml +++ b/doc/src/sgml/ref/reindex.sgml @@ -25,7 +25,7 @@ REINDEX [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] { IN <phrase>where <replaceable class="parameter">option</replaceable> can be one of:</phrase> - VERBOSE + VERBOSE [ <replaceable class="parameter">boolean</replaceable> ] </synopsis> </refsynopsisdiv> @@ -141,19 +141,6 @@ REINDEX [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] { IN </listitem> </varlistentry> - <varlistentry> - <term><replaceable class="parameter">name</replaceable></term> - <listitem> - <para> - The name of the specific index, table, or database to be - reindexed. Index and table names can be schema-qualified. - Presently, <command>REINDEX DATABASE</command> and <command>REINDEX SYSTEM</command> - can only reindex the current database, so their parameter must match - the current database's name. - </para> - </listitem> - </varlistentry> - <varlistentry> <term><literal>CONCURRENTLY</literal></term> <listitem> @@ -181,6 +168,34 @@ REINDEX [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] { IN </para> </listitem> </varlistentry> + + <varlistentry> + <term><replaceable class="parameter">name</replaceable></term> + <listitem> + <para> + The name of the specific index, table, or database to be + reindexed. Index and table names can be schema-qualified. + Presently, <command>REINDEX DATABASE</command> and <command>REINDEX SYSTEM</command> + can only reindex the current database, so their parameter must match + the current database's name. + </para> + </listitem> + </varlistentry> + + <varlistentry> + <term><replaceable class="parameter">boolean</replaceable></term> + <listitem> + <para> + Specifies whether the selected option should be turned on or off. + You can write <literal>TRUE</literal>, <literal>ON</literal>, or + <literal>1</literal> to enable the option, and <literal>FALSE</literal>, + <literal>OFF</literal>, or <literal>0</literal> to disable it. The + <replaceable class="parameter">boolean</replaceable> value can also + be omitted, in which case <literal>TRUE</literal> is assumed. + </para> + </listitem> + </varlistentry> + </variablelist> </refsect1> diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c index 04d12a7ece..1817b8829a 100644 --- a/src/backend/commands/cluster.c +++ b/src/backend/commands/cluster.c @@ -35,6 +35,7 @@ #include "catalog/pg_am.h" #include "catalog/toasting.h" #include "commands/cluster.h" +#include "commands/defrem.h" #include "commands/progress.h" #include "commands/tablecmds.h" #include "commands/vacuum.h" @@ -99,8 +100,28 @@ static List *get_tables_to_cluster(MemoryContext cluster_context); *--------------------------------------------------------------------------- */ void -cluster(ClusterStmt *stmt, bool isTopLevel) +cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel) { + ListCell *lc; + + /* Parse list of generic parameters not handled by the parser */ + foreach(lc, stmt->params) + { + DefElem *opt = (DefElem *) lfirst(lc); + + if (strcmp(opt->defname, "verbose") == 0) + if (defGetBoolean(opt)) + stmt->options |= CLUOPT_VERBOSE; + else + stmt->options &= ~CLUOPT_VERBOSE; + else + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("unrecognized CLUSTER option \"%s\"", + opt->defname), + parser_errposition(pstate, opt->location))); + } + if (stmt->relation != NULL) { /* This is the single-relation case. */ diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index 7819266a63..a22b32fc74 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -2420,8 +2420,9 @@ ChooseIndexColumnNames(List *indexElems) * Recreate a specific index. */ void -ReindexIndex(RangeVar *indexRelation, int options, bool concurrent) +ReindexIndex(ReindexStmt *stmt) { + RangeVar *indexRelation = stmt->relation; struct ReindexIndexCallbackState state; Oid indOid; Relation irel; @@ -2437,10 +2438,10 @@ ReindexIndex(RangeVar *indexRelation, int options, bool concurrent) * upgrade the lock, but that's OK, because other sessions can't hold * locks on our temporary table. */ - state.concurrent = concurrent; + state.concurrent = stmt->concurrent; state.locked_table_oid = InvalidOid; indOid = RangeVarGetRelidExtended(indexRelation, - concurrent ? ShareUpdateExclusiveLock : AccessExclusiveLock, + stmt->concurrent ? ShareUpdateExclusiveLock : AccessExclusiveLock, 0, RangeVarCallbackForReindexIndex, &state); @@ -2460,11 +2461,11 @@ ReindexIndex(RangeVar *indexRelation, int options, bool concurrent) persistence = irel->rd_rel->relpersistence; index_close(irel, NoLock); - if (concurrent && persistence != RELPERSISTENCE_TEMP) - ReindexRelationConcurrently(indOid, options); + if (stmt->concurrent && persistence != RELPERSISTENCE_TEMP) + ReindexRelationConcurrently(indOid, stmt->options); else reindex_index(indOid, false, persistence, - options | REINDEXOPT_REPORT_PROGRESS); + stmt->options | REINDEXOPT_REPORT_PROGRESS); } /* @@ -2542,8 +2543,9 @@ RangeVarCallbackForReindexIndex(const RangeVar *relation, * Recreate all indexes of a table (and of its toast table, if any) */ Oid -ReindexTable(RangeVar *relation, int options, bool concurrent) +ReindexTable(ReindexStmt *stmt) { + RangeVar *relation = stmt->relation; Oid heapOid; bool result; @@ -2556,13 +2558,13 @@ ReindexTable(RangeVar *relation, int options, bool concurrent) * locks on our temporary table. */ heapOid = RangeVarGetRelidExtended(relation, - concurrent ? ShareUpdateExclusiveLock : ShareLock, + stmt->concurrent ? ShareUpdateExclusiveLock : ShareLock, 0, RangeVarCallbackOwnsTable, NULL); - if (concurrent && get_rel_persistence(heapOid) != RELPERSISTENCE_TEMP) + if (stmt->concurrent && get_rel_persistence(heapOid) != RELPERSISTENCE_TEMP) { - result = ReindexRelationConcurrently(heapOid, options); + result = ReindexRelationConcurrently(heapOid, stmt->options); if (!result) ereport(NOTICE, @@ -2574,7 +2576,7 @@ ReindexTable(RangeVar *relation, int options, bool concurrent) result = reindex_relation(heapOid, REINDEX_REL_PROCESS_TOAST | REINDEX_REL_CHECK_CONSTRAINTS, - options | REINDEXOPT_REPORT_PROGRESS); + stmt->options | REINDEXOPT_REPORT_PROGRESS); if (!result) ereport(NOTICE, (errmsg("table \"%s\" has no indexes to reindex", @@ -2593,9 +2595,10 @@ ReindexTable(RangeVar *relation, int options, bool concurrent) * That means this must not be called within a user transaction block! */ void -ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind, - int options, bool concurrent) +ReindexMultipleTables(ReindexStmt *stmt) { + const char *objectName = stmt->name; + ReindexObjectType objectKind = stmt->kind; Oid objectOid; Relation relationRelation; TableScanDesc scan; @@ -2613,7 +2616,7 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind, objectKind == REINDEX_OBJECT_SYSTEM || objectKind == REINDEX_OBJECT_DATABASE); - if (objectKind == REINDEX_OBJECT_SYSTEM && concurrent) + if (objectKind == REINDEX_OBJECT_SYSTEM && stmt->concurrent) ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), errmsg("cannot reindex system catalogs concurrently"))); @@ -2724,7 +2727,7 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind, * Skip system tables, since index_create() would reject indexing them * concurrently (and it would likely fail if we tried). */ - if (concurrent && + if (stmt->concurrent && IsCatalogRelationOid(relid)) { if (!concurrent_warning) @@ -2766,9 +2769,9 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind, /* functions in indexes may want a snapshot set */ PushActiveSnapshot(GetTransactionSnapshot()); - if (concurrent && get_rel_persistence(relid) != RELPERSISTENCE_TEMP) + if (stmt->concurrent && get_rel_persistence(relid) != RELPERSISTENCE_TEMP) { - (void) ReindexRelationConcurrently(relid, options); + (void) ReindexRelationConcurrently(relid, stmt->options); /* ReindexRelationConcurrently() does the verbose output */ } else @@ -2778,9 +2781,9 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind, result = reindex_relation(relid, REINDEX_REL_PROCESS_TOAST | REINDEX_REL_CHECK_CONSTRAINTS, - options | REINDEXOPT_REPORT_PROGRESS); + stmt->options | REINDEXOPT_REPORT_PROGRESS); - if (result && (options & REINDEXOPT_VERBOSE)) + if (result && (stmt->options & REINDEXOPT_VERBOSE)) ereport(INFO, (errmsg("table \"%s.%s\" was reindexed", get_namespace_name(get_rel_namespace(relid)), diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c index 89c409de66..71548acc0c 100644 --- a/src/backend/nodes/copyfuncs.c +++ b/src/backend/nodes/copyfuncs.c @@ -3358,6 +3358,7 @@ _copyClusterStmt(const ClusterStmt *from) COPY_NODE_FIELD(relation); COPY_STRING_FIELD(indexname); COPY_SCALAR_FIELD(options); + COPY_NODE_FIELD(params); return newnode; } @@ -4450,6 +4451,7 @@ _copyReindexStmt(const ReindexStmt *from) COPY_NODE_FIELD(relation); COPY_STRING_FIELD(name); COPY_SCALAR_FIELD(options); + COPY_NODE_FIELD(params); COPY_SCALAR_FIELD(concurrent); return newnode; diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c index e3f33c40be..de48a42cdd 100644 --- a/src/backend/nodes/equalfuncs.c +++ b/src/backend/nodes/equalfuncs.c @@ -1217,6 +1217,7 @@ _equalClusterStmt(const ClusterStmt *a, const ClusterStmt *b) COMPARE_NODE_FIELD(relation); COMPARE_STRING_FIELD(indexname); COMPARE_SCALAR_FIELD(options); + COMPARE_NODE_FIELD(params); return true; } @@ -2135,6 +2136,7 @@ _equalReindexStmt(const ReindexStmt *a, const ReindexStmt *b) COMPARE_NODE_FIELD(relation); COMPARE_STRING_FIELD(name); COMPARE_SCALAR_FIELD(options); + COMPARE_NODE_FIELD(params); COMPARE_SCALAR_FIELD(concurrent); return true; diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y index dbb47d4982..b909b161a6 100644 --- a/src/backend/parser/gram.y +++ b/src/backend/parser/gram.y @@ -519,7 +519,6 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); %type <list> explain_option_list %type <ival> reindex_target_type reindex_target_multitable -%type <ival> reindex_option_list reindex_option_elem %type <node> copy_generic_opt_arg copy_generic_opt_arg_list_item %type <defelt> copy_generic_opt_elem @@ -8180,7 +8179,7 @@ ReindexStmt: n->concurrent = $3; n->relation = $4; n->name = NULL; - n->options = 0; + n->params = NIL; $$ = (Node *)n; } | REINDEX reindex_target_multitable opt_concurrently name @@ -8190,27 +8189,27 @@ ReindexStmt: n->concurrent = $3; n->name = $4; n->relation = NULL; - n->options = 0; + n->params = NIL; $$ = (Node *)n; } - | REINDEX '(' reindex_option_list ')' reindex_target_type opt_concurrently qualified_name + | REINDEX '(' vac_analyze_option_list ')' reindex_target_type opt_concurrently qualified_name { ReindexStmt *n = makeNode(ReindexStmt); n->kind = $5; n->concurrent = $6; n->relation = $7; n->name = NULL; - n->options = $3; + n->params = $3; $$ = (Node *)n; } - | REINDEX '(' reindex_option_list ')' reindex_target_multitable opt_concurrently name + | REINDEX '(' vac_analyze_option_list ')' reindex_target_multitable opt_concurrently name { ReindexStmt *n = makeNode(ReindexStmt); n->kind = $5; n->concurrent = $6; n->name = $7; n->relation = NULL; - n->options = $3; + n->params = $3; $$ = (Node *)n; } ; @@ -8223,13 +8222,6 @@ reindex_target_multitable: | SYSTEM_P { $$ = REINDEX_OBJECT_SYSTEM; } | DATABASE { $$ = REINDEX_OBJECT_DATABASE; } ; -reindex_option_list: - reindex_option_elem { $$ = $1; } - | reindex_option_list ',' reindex_option_elem { $$ = $1 | $3; } - ; -reindex_option_elem: - VERBOSE { $$ = REINDEXOPT_VERBOSE; } - ; /***************************************************************************** * @@ -10380,6 +10372,7 @@ CreateConversionStmt: * * QUERY: * CLUSTER [VERBOSE] <qualified_name> [ USING <index_name> ] + * CLUSTER [VERBOSE] [(options)] <qualified_name> [ USING <index_name> ] * CLUSTER [VERBOSE] * CLUSTER [VERBOSE] <index_name> ON <qualified_name> (for pre-8.3) * @@ -10394,6 +10387,18 @@ ClusterStmt: n->options = 0; if ($2) n->options |= CLUOPT_VERBOSE; + n->params = NIL; + $$ = (Node*)n; + } + + | CLUSTER opt_verbose '(' vac_analyze_option_list ')' qualified_name cluster_index_specification + { + ClusterStmt *n = makeNode(ClusterStmt); + n->relation = $6; + n->indexname = $7; + if ($2) + n->options |= CLUOPT_VERBOSE; + n->params = $4; $$ = (Node*)n; } | CLUSTER opt_verbose @@ -10404,6 +10409,7 @@ ClusterStmt: n->options = 0; if ($2) n->options |= CLUOPT_VERBOSE; + n->params = NIL; $$ = (Node*)n; } /* kept for pre-8.3 compatibility */ @@ -10415,6 +10421,7 @@ ClusterStmt: n->options = 0; if ($2) n->options |= CLUOPT_VERBOSE; + n->params = NIL; $$ = (Node*)n; } ; diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c index 9b0c376c8c..c48983a5de 100644 --- a/src/backend/tcop/utility.c +++ b/src/backend/tcop/utility.c @@ -526,6 +526,33 @@ ProcessUtility(PlannedStmt *pstmt, dest, qc); } +/* Parse params not parsed by the grammar */ +static +void parse_reindex_params(ParseState *pstate, ReindexStmt *stmt) +{ + ListCell *lc; + foreach(lc, stmt->params) + { + DefElem *opt = (DefElem *) lfirst(lc); + + if (strcmp(opt->defname, "verbose") == 0) + { + if (defGetBoolean(opt)) + stmt->options |= REINDEXOPT_VERBOSE; + else + stmt->options &= ~REINDEXOPT_VERBOSE; + } + else if (strcmp(opt->defname, "concurrently") == 0) + stmt->concurrent = true; + else + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("unrecognized REINDEX option \"%s\"", + opt->defname), + parser_errposition(pstate, opt->location))); + } +} + /* * standard_ProcessUtility itself deals only with utility commands for * which we do not provide event trigger support. Commands that do have @@ -818,7 +845,7 @@ standard_ProcessUtility(PlannedStmt *pstmt, break; case T_ClusterStmt: - cluster((ClusterStmt *) parsetree, isTopLevel); + cluster(pstate, (ClusterStmt *) parsetree, isTopLevel); break; case T_VacuumStmt: @@ -923,13 +950,14 @@ standard_ProcessUtility(PlannedStmt *pstmt, PreventInTransactionBlock(isTopLevel, "REINDEX CONCURRENTLY"); + parse_reindex_params(pstate, stmt); switch (stmt->kind) { case REINDEX_OBJECT_INDEX: - ReindexIndex(stmt->relation, stmt->options, stmt->concurrent); + ReindexIndex(stmt); break; case REINDEX_OBJECT_TABLE: - ReindexTable(stmt->relation, stmt->options, stmt->concurrent); + ReindexTable(stmt); break; case REINDEX_OBJECT_SCHEMA: case REINDEX_OBJECT_SYSTEM: @@ -945,7 +973,7 @@ standard_ProcessUtility(PlannedStmt *pstmt, (stmt->kind == REINDEX_OBJECT_SCHEMA) ? "REINDEX SCHEMA" : (stmt->kind == REINDEX_OBJECT_SYSTEM) ? "REINDEX SYSTEM" : "REINDEX DATABASE"); - ReindexMultipleTables(stmt->name, stmt->kind, stmt->options, stmt->concurrent); + ReindexMultipleTables(stmt); break; default: elog(ERROR, "unrecognized object type: %d", diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c index f41785f11c..de85772ff9 100644 --- a/src/bin/psql/tab-complete.c +++ b/src/bin/psql/tab-complete.c @@ -2259,21 +2259,34 @@ psql_completion(const char *text, int start, int end) /* CLUSTER */ else if (Matches("CLUSTER")) COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_clusterables, "UNION SELECT 'VERBOSE'"); - else if (Matches("CLUSTER", "VERBOSE")) + else if (Matches("CLUSTER", "VERBOSE") || + Matches("CLUSTER", "VERBOSE", "(*)") || + Matches("CLUSTER", "(*)")) COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_clusterables, NULL); /* If we have CLUSTER <sth>, then add "USING" */ - else if (Matches("CLUSTER", MatchAnyExcept("VERBOSE|ON"))) + else if (Matches("CLUSTER", MatchAnyExcept("VERBOSE|ON|(|(*)"))) COMPLETE_WITH("USING"); /* If we have CLUSTER VERBOSE <sth>, then add "USING" */ - else if (Matches("CLUSTER", "VERBOSE", MatchAny)) + else if (Matches("CLUSTER", "VERBOSE|(*)", MatchAny)) COMPLETE_WITH("USING"); /* If we have CLUSTER <sth> USING, then add the index as well */ else if (Matches("CLUSTER", MatchAny, "USING") || - Matches("CLUSTER", "VERBOSE", MatchAny, "USING")) + Matches("CLUSTER", "VERBOSE|(*)", MatchAny, "USING")) { completion_info_charp = prev2_wd; COMPLETE_WITH_QUERY(Query_for_index_of_table); } + else if (HeadMatches("CLUSTER", "(*") && + !HeadMatches("CLUSTER", "(*)")) + { + /* + * This fires if we're in an unfinished parenthesized option list. + * get_previous_words treats a completed parenthesized option list as + * one word, so the above test is correct. + */ + if (ends_with(prev_wd, '(') || ends_with(prev_wd, ',')) + COMPLETE_WITH("VERBOSE"); + } /* COMMENT */ else if (Matches("COMMENT")) @@ -3470,7 +3483,7 @@ psql_completion(const char *text, int start, int end) * one word, so the above test is correct. */ if (ends_with(prev_wd, '(') || ends_with(prev_wd, ',')) - COMPLETE_WITH("VERBOSE"); + COMPLETE_WITH("CONCURRENTLY", "VERBOSE"); } /* SECURITY LABEL */ diff --git a/src/include/commands/cluster.h b/src/include/commands/cluster.h index e05884781b..674cdcd0cd 100644 --- a/src/include/commands/cluster.h +++ b/src/include/commands/cluster.h @@ -14,11 +14,12 @@ #define CLUSTER_H #include "nodes/parsenodes.h" +#include "parser/parse_node.h" #include "storage/lock.h" #include "utils/relcache.h" -extern void cluster(ClusterStmt *stmt, bool isTopLevel); +extern void cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel); extern void cluster_rel(Oid tableOid, Oid indexOid, int options); extern void check_index_is_clusterable(Relation OldHeap, Oid indexOid, bool recheck, LOCKMODE lockmode); diff --git a/src/include/commands/defrem.h b/src/include/commands/defrem.h index c26a102b17..a97c00a02e 100644 --- a/src/include/commands/defrem.h +++ b/src/include/commands/defrem.h @@ -34,10 +34,9 @@ extern ObjectAddress DefineIndex(Oid relationId, bool check_not_in_use, bool skip_build, bool quiet); -extern void ReindexIndex(RangeVar *indexRelation, int options, bool concurrent); -extern Oid ReindexTable(RangeVar *relation, int options, bool concurrent); -extern void ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind, - int options, bool concurrent); +extern void ReindexIndex(ReindexStmt *stmt); +extern Oid ReindexTable(ReindexStmt *stmt); +extern void ReindexMultipleTables(ReindexStmt *stmt); extern char *makeObjectName(const char *name1, const char *name2, const char *label); extern char *ChooseRelationName(const char *name1, const char *name2, diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h index 151bcdb7ef..295c2802a4 100644 --- a/src/include/nodes/parsenodes.h +++ b/src/include/nodes/parsenodes.h @@ -3210,6 +3210,7 @@ typedef struct ClusterStmt RangeVar *relation; /* relation being indexed, or NULL if all */ char *indexname; /* original index defined */ int options; /* OR of ClusterOption flags */ + List *params; /* Params not further parsed by the grammar */ } ClusterStmt; /* ---------------------- @@ -3369,6 +3370,7 @@ typedef struct ReindexStmt RangeVar *relation; /* Table or index to reindex */ const char *name; /* name of database to reindex */ int options; /* Reindex options flags */ + List *params; /* Params not further parsed by the grammer */ bool concurrent; /* reindex concurrently? */ } ReindexStmt; -- 2.17.0 --g7w8+K/95kPelPD2 Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v22-0002-Deprecate-ReindexStmt-concurrent.patch" ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: memory leak in trigger handling (since PG12) @ 2023-05-23 17:28 Tom Lane <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Tom Lane @ 2023-05-23 17:28 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Tomas Vondra <[email protected]>; PostgreSQL Hackers <[email protected]> Andres Freund <[email protected]> writes: > I've wondered about some form of instrumentation to detect such issues > before. Yeah. > Could it help to have a mode where the executor shutdown hook checks how much > memory is allocated in ExecutorState and warns if its too much? It'd be very hard to set a limit for what's "too much", since the amount of stuff created initially will depend on the plan size. In any case I think that the important issue is not how much absolute space, but is there per-row leakage. I wonder if we could do something involving checking for continued growth after the first retrieved tuple, or something like that. > Random aside: I've been wondering whether it'd be worth introducing an > in-place representation of Bitmap (e.g. if the low bit is set, the low 63 bits > are in-place, if unset, it's a pointer). Why? Unlike Lists, those things are already a single palloc chunk. regards, tom lane ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: memory leak in trigger handling (since PG12) @ 2023-05-23 18:01 Andres Freund <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Andres Freund @ 2023-05-23 18:01 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Tomas Vondra <[email protected]>; PostgreSQL Hackers <[email protected]> Hi, On 2023-05-23 13:28:30 -0400, Tom Lane wrote: > Andres Freund <[email protected]> writes: > > Could it help to have a mode where the executor shutdown hook checks how much > > memory is allocated in ExecutorState and warns if its too much? > > It'd be very hard to set a limit for what's "too much", since the amount > of stuff created initially will depend on the plan size. I was thinking of some limit that should really never be reached outside of a leak or work_mem based allocations, say 2GB or so. > In any case I think that the important issue is not how much absolute space, > but is there per-row leakage. I wonder if we could do something involving > checking for continued growth after the first retrieved tuple, or something > like that. As-is I some nodes do large allocations the query context, but are not guaranteed to be reached when gathering the first row. So we would still have to move such large allocations out of ExecutorState. I think it might be best to go for a combination of these two heuristics. Store the size of es_query_context after standard_ExecutorStart(), that would include the allocation of the executor tree itself. Then in standard_ExecutorEnd(), if the difference in size of ExecutorState is bigger than some constant *and* is bigger than the initial size by some factor, emit a warning. The constant size difference avoids spurious warnings in case of a small plan that just grows due to a few fmgr lookups or such, the factor takes care of the plan complexity? > > Random aside: I've been wondering whether it'd be worth introducing an > > in-place representation of Bitmap (e.g. if the low bit is set, the low 63 bits > > are in-place, if unset, it's a pointer). > > Why? Unlike Lists, those things are already a single palloc chunk. We do a fair amount of 8 byte allocations - they have quite a bit of overhead, even after c6e0fe1f2a0. Not needing allocations for the common case of bitmapsets with a max member < 63 seems like it could be worth it. Greetings, Andres Freund ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: memory leak in trigger handling (since PG12) @ 2023-05-24 08:19 Jakub Wartak <[email protected]> parent: Andres Freund <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Jakub Wartak @ 2023-05-24 08:19 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Tom Lane <[email protected]>; Tomas Vondra <[email protected]>; PostgreSQL Hackers <[email protected]> Hi, just two cents: On Tue, May 23, 2023 at 8:01 PM Andres Freund <[email protected]> wrote: > > Hi, > > On 2023-05-23 13:28:30 -0400, Tom Lane wrote: > > Andres Freund <[email protected]> writes: > > > Could it help to have a mode where the executor shutdown hook checks how much > > > memory is allocated in ExecutorState and warns if its too much? > > > > It'd be very hard to set a limit for what's "too much", since the amount > > of stuff created initially will depend on the plan size. > > I was thinking of some limit that should really never be reached outside of a > leak or work_mem based allocations, say 2GB or so. RE: instrumentation subthread: if that helps then below technique can work somewhat good on normal binaries for end users (given there are debug symbols installed), so maybe we don't need that much infrastructure added in to see the hot code path: perf probe -x /path/to/postgres 'palloc' 'size=%di:u64' # RDI on x86_64(palloc size arg0) perf record -avg --call-graph dwarf -e probe_postgres:palloc -aR -p <pid> sleep 3 # cannot be longer, huge overhead (~3s=~2GB) it produces: 50.27% (563d0e380670) size=24 | ---palloc bms_copy ExecUpdateLockMode ExecBRUpdateTriggers ExecUpdate [..] 49.73% (563d0e380670) size=16 | ---palloc bms_copy RelationGetIndexAttrBitmap ExecUpdateLockMode ExecBRUpdateTriggers ExecUpdate [..] Now we know that those small palloc() are guilty, but we didn't know at the time with Tomas. The problem here is that we do not know in palloc() - via its own arguments for which MemoryContext this is going to be allocated for. This is problematic for perf, because on RHEL8, I was not able to generate an uprobe that was capable of reaching a global variable (CurrentMemoryContext) at that time. Additionally what was even more frustrating on diagnosing that case on the customer end system, was that such OOMs were crashing other PostgreSQL clusters on the same OS. Even knowing the exact guilty statement it was impossible to limit RSS memory usage of that particular backend. So, what you are proposing makes a lot of sense. Also it got me thinking of implementing safety-memory-net-GUC debug_query_limit_backend_memory=X MB that would inject setrlimit(RLIMIT_DATA) through external extension via hook(s) and un-set it later, but the man page states it works for mmap() only after Linux 4.7+ so it is future proof but won't work e.g. on RHEL7 - maybe that's still good enough?; Or, well maybe try to hack a palloc() a little, but that has probably too big overhead, right? (just thinking loud). -Jakub Wartak. ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: memory leak in trigger handling (since PG12) @ 2023-05-24 13:17 Tomas Vondra <[email protected]> parent: Jakub Wartak <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: Tomas Vondra @ 2023-05-24 13:17 UTC (permalink / raw) To: Jakub Wartak <[email protected]>; Andres Freund <[email protected]>; +Cc: Tom Lane <[email protected]>; PostgreSQL Hackers <[email protected]> On 5/24/23 10:19, Jakub Wartak wrote: > Hi, just two cents: > > On Tue, May 23, 2023 at 8:01 PM Andres Freund <[email protected]> wrote: >> >> Hi, >> >> On 2023-05-23 13:28:30 -0400, Tom Lane wrote: >>> Andres Freund <[email protected]> writes: >>>> Could it help to have a mode where the executor shutdown hook checks how much >>>> memory is allocated in ExecutorState and warns if its too much? >>> >>> It'd be very hard to set a limit for what's "too much", since the amount >>> of stuff created initially will depend on the plan size. >> >> I was thinking of some limit that should really never be reached outside of a >> leak or work_mem based allocations, say 2GB or so. > > RE: instrumentation subthread: > if that helps then below technique can work somewhat good on normal > binaries for end users (given there are debug symbols installed), so > maybe we don't need that much infrastructure added in to see the hot > code path: > > perf probe -x /path/to/postgres 'palloc' 'size=%di:u64' # RDI on > x86_64(palloc size arg0) > perf record -avg --call-graph dwarf -e probe_postgres:palloc -aR -p > <pid> sleep 3 # cannot be longer, huge overhead (~3s=~2GB) > > it produces: > 50.27% (563d0e380670) size=24 > | > ---palloc > bms_copy > ExecUpdateLockMode > ExecBRUpdateTriggers > ExecUpdate > [..] > > 49.73% (563d0e380670) size=16 > | > ---palloc > bms_copy > RelationGetIndexAttrBitmap > ExecUpdateLockMode > ExecBRUpdateTriggers > ExecUpdate > [..] > > Now we know that those small palloc() are guilty, but we didn't know > at the time with Tomas. The problem here is that we do not know in > palloc() - via its own arguments for which MemoryContext this is going > to be allocated for. This is problematic for perf, because on RHEL8, I > was not able to generate an uprobe that was capable of reaching a > global variable (CurrentMemoryContext) at that time. > I think there are a couple even bigger issues: (a) There are other methods that allocate memory - repalloc, palloc0, MemoryContextAlloc, ... and so on. But presumably we can trace all of them at once? We'd have to trace reset/deletes too. (b) This doesn't say if/when the allocated chunks get freed - even with a fix, we'll still do exactly the same number of allocations, except that we'll free the memory shortly after that. I wonder if we could print a bit more info for each probe, to match the alloc/free requests. > Additionally what was even more frustrating on diagnosing that case on > the customer end system, was that such OOMs were crashing other > PostgreSQL clusters on the same OS. Even knowing the exact guilty > statement it was impossible to limit RSS memory usage of that > particular backend. So, what you are proposing makes a lot of sense. > Also it got me thinking of implementing safety-memory-net-GUC > debug_query_limit_backend_memory=X MB that would inject > setrlimit(RLIMIT_DATA) through external extension via hook(s) and > un-set it later, but the man page states it works for mmap() only > after Linux 4.7+ so it is future proof but won't work e.g. on RHEL7 - > maybe that's still good enough?; Or, well maybe try to hack a palloc() > a little, but that has probably too big overhead, right? (just > thinking loud). > Not sure about setting a hard limit - that seems pretty tricky and may easily backfire. It's already possible to set such memory limit using e.g. cgroups, or even better use VMs to isolate the instances. I certainly agree it's annoying that when OOM hits, we end up with no information about what used the memory. Maybe we could have a threshold triggering a call to MemoryContextStats? So that we have at least some memory usage info in the log in case the OOM killer intervenes. regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company ^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2023-05-24 13:17 UTC | newest] Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-03-27 22:50 [PATCH v22 1/5] Change REINDEX/CLUSTER to accept an option list.. Justin Pryzby <[email protected]> 2023-05-23 17:28 Re: memory leak in trigger handling (since PG12) Tom Lane <[email protected]> 2023-05-23 18:01 ` Re: memory leak in trigger handling (since PG12) Andres Freund <[email protected]> 2023-05-24 08:19 ` Re: memory leak in trigger handling (since PG12) Jakub Wartak <[email protected]> 2023-05-24 13:17 ` Re: memory leak in trigger handling (since PG12) Tomas Vondra <[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