public inbox for [email protected]help / color / mirror / Atom feed
Re: GUCs to control abbreviated sort keys 2+ messages / 2 participants [nested] [flat]
* Re: GUCs to control abbreviated sort keys @ 2023-01-25 21:30 Jeff Davis <[email protected]> 0 siblings, 0 replies; 2+ messages in thread From: Jeff Davis @ 2023-01-25 21:30 UTC (permalink / raw) To: Justin Pryzby <[email protected]>; +Cc: pgsql-hackers On Tue, 2023-01-24 at 19:43 -0600, Justin Pryzby wrote: > I think "an optimization, if applicable" is either too terse, or > somehow > wrong. Maybe: > > > Enables or disables the use of abbreviated keys, a sort > > optimization... Done. > > + optimization could return wrong results. Set to > > + <literal>true</literal> if certain that > > <function>strxfrm()</function> > > + can be trusted. > > "if you are certain"; or "if it is ..." Done. Thank you, rebased patch attached. -- Jeff Davis PostgreSQL Contributor Team - AWS Attachments: [text/x-patch] v2-0001-Introduce-GUCs-to-control-abbreviated-keys-sort-o.patch (8.4K, ../../[email protected]/2-v2-0001-Introduce-GUCs-to-control-abbreviated-keys-sort-o.patch) download | inline diff: From 39ed011cc51ba3a4af5e3b559a7b8de25fb895a5 Mon Sep 17 00:00:00 2001 From: Jeff Davis <[email protected]> Date: Sat, 21 Jan 2023 12:44:07 -0800 Subject: [PATCH v2] Introduce GUCs to control abbreviated keys sort optimization. The setting sort_abbreviated_keys turns the optimization on or off overall. The optimization relies on collation providers, which are complex dependencies, and the performance of the optimization may rely on many factors. Introducing a GUC allows easier diagnosis when this optimization results in worse perforamnce. The setting trust_strxfrm replaces the define TRUST_STRXFRM, allowing users to experiment with the abbreviated keys optimization when using the libc provider. Previously, the optimization only applied to collations using the ICU provider unless specially compiled. By default, allowed only for superusers (because an incorrect setting could lead to wrong results), but can be granted to others. --- doc/src/sgml/config.sgml | 40 ++++++++++++++++++++++ src/backend/utils/adt/varlena.c | 10 +++--- src/backend/utils/misc/guc_tables.c | 24 +++++++++++++ src/backend/utils/sort/tuplesortvariants.c | 17 ++++++--- 4 files changed, 82 insertions(+), 9 deletions(-) diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index f985afc009..8f55b89f35 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -11252,6 +11252,46 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir' </listitem> </varlistentry> + <varlistentry id="guc-sort-abbreviated-keys" xreflabel="sort_abbreviated_keys"> + <term><varname>sort_abbreviated_keys</varname> (<type>boolean</type>) + <indexterm> + <primary><varname>sort_abbreviated_keys</varname> configuration parameter</primary> + </indexterm> + </term> + <listitem> + <para> + Enables or disables the use of abbreviated sort keys, a sort optimization, + if applicable. The default is <literal>true</literal>. Disabling may + be useful to diagnose problems or measure performance. + </para> + </listitem> + </varlistentry> + + <varlistentry id="guc-trust-strxfrm" xreflabel="trust_strxfrm"> + <term><varname>trust_strxfrm</varname> (<type>boolean</type>) + <indexterm> + <primary><varname>trust_strxfrm</varname> configuration parameter</primary> + </indexterm> + </term> + <listitem> + <para> + Abbreviated keys, a sort optimization, depends on correct behavior of + the operating system function <function>strxfrm()</function> when + using a collation with the <literal>libc</literal> provider. On some + platforms <function>strxfrm()</function> does not return results + consistent with <function>strcoll()</function>, which means the + optimization could return wrong results. Set to + <literal>true</literal> if it is certain that + <function>strxfrm()</function> can be trusted. + </para> + <para> + The default value is <literal>false</literal>. This setting has no + effect if <xref linkend="guc-sort-abbreviated-keys"/> is set to + <literal>false</literal>. + </para> + </listitem> + </varlistentry> + <varlistentry id="guc-trace-locks" xreflabel="trace_locks"> <term><varname>trace_locks</varname> (<type>boolean</type>) <indexterm> diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c index fd81c47474..c270022483 100644 --- a/src/backend/utils/adt/varlena.c +++ b/src/backend/utils/adt/varlena.c @@ -45,6 +45,9 @@ /* GUC variable */ int bytea_output = BYTEA_OUTPUT_HEX; +/* GUC to enable use of strxfrm() for abbreviated keys */ +bool trust_strxfrm = false; + typedef struct varlena unknown; typedef struct varlena VarString; @@ -2115,7 +2118,7 @@ varstr_sortsupport(SortSupport ssup, Oid typid, Oid collid) * other libc other than Cygwin has so far been shown to have a problem, * we take the conservative course of action for right now and disable * this categorically. (Users who are certain this isn't a problem on - * their system can define TRUST_STRXFRM.) + * their system can set the trust_strxfrm GUC to true.) * * Even apart from the risk of broken locales, it's possible that there * are platforms where the use of abbreviated keys should be disabled at @@ -2128,10 +2131,9 @@ varstr_sortsupport(SortSupport ssup, Oid typid, Oid collid) * categorically, we may still want or need to disable it for particular * platforms. */ -#ifndef TRUST_STRXFRM - if (!collate_c && !(locale && locale->provider == COLLPROVIDER_ICU)) + if (!trust_strxfrm && !collate_c && + !(locale && locale->provider == COLLPROVIDER_ICU)) abbreviate = false; -#endif /* * If we're using abbreviated keys, or if we're using a locale-aware diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c index 4ac808ed22..fd4a02fbf5 100644 --- a/src/backend/utils/misc/guc_tables.c +++ b/src/backend/utils/misc/guc_tables.c @@ -102,6 +102,8 @@ extern bool trace_syncscan; #ifdef DEBUG_BOUNDED_SORT extern bool optimize_bounded_sort; #endif +extern bool sort_abbreviated_keys; +extern bool trust_strxfrm; /* * Options for enum values defined in this module. @@ -1673,6 +1675,28 @@ struct config_bool ConfigureNamesBool[] = }, #endif + { + {"sort_abbreviated_keys", PGC_USERSET, DEVELOPER_OPTIONS, + gettext_noop("Enables the use of abbreviated sort keys."), + NULL, + GUC_NOT_IN_SAMPLE | GUC_EXPLAIN + }, + &sort_abbreviated_keys, + true, + NULL, NULL, NULL + }, + + { + {"trust_strxfrm", PGC_SUSET, DEVELOPER_OPTIONS, + gettext_noop("Allow use of strxfrm() for abbreviated keys optimization for libc provider."), + NULL, + GUC_NOT_IN_SAMPLE + }, + &trust_strxfrm, + false, + NULL, NULL, NULL + }, + #ifdef WAL_DEBUG { {"wal_debug", PGC_SUSET, DEVELOPER_OPTIONS, diff --git a/src/backend/utils/sort/tuplesortvariants.c b/src/backend/utils/sort/tuplesortvariants.c index eb6cfcfd00..ba16779f97 100644 --- a/src/backend/utils/sort/tuplesortvariants.c +++ b/src/backend/utils/sort/tuplesortvariants.c @@ -37,6 +37,8 @@ #define DATUM_SORT 2 #define CLUSTER_SORT 3 +bool sort_abbreviated_keys = true; + static void removeabbrev_heap(Tuplesortstate *state, SortTuple *stups, int count); static void removeabbrev_cluster(Tuplesortstate *state, SortTuple *stups, @@ -185,7 +187,8 @@ tuplesort_begin_heap(TupleDesc tupDesc, sortKey->ssup_nulls_first = nullsFirstFlags[i]; sortKey->ssup_attno = attNums[i]; /* Convey if abbreviation optimization is applicable in principle */ - sortKey->abbreviate = (i == 0 && base->haveDatum1); + if (sort_abbreviated_keys) + sortKey->abbreviate = (i == 0 && base->haveDatum1); PrepareSortSupportFromOrderingOp(sortOperators[i], sortKey); } @@ -295,7 +298,8 @@ tuplesort_begin_cluster(TupleDesc tupDesc, (scanKey->sk_flags & SK_BT_NULLS_FIRST) != 0; sortKey->ssup_attno = scanKey->sk_attno; /* Convey if abbreviation optimization is applicable in principle */ - sortKey->abbreviate = (i == 0 && base->haveDatum1); + if (sort_abbreviated_keys) + sortKey->abbreviate = (i == 0 && base->haveDatum1); Assert(sortKey->ssup_attno != 0); @@ -379,7 +383,8 @@ tuplesort_begin_index_btree(Relation heapRel, (scanKey->sk_flags & SK_BT_NULLS_FIRST) != 0; sortKey->ssup_attno = scanKey->sk_attno; /* Convey if abbreviation optimization is applicable in principle */ - sortKey->abbreviate = (i == 0 && base->haveDatum1); + if (sort_abbreviated_keys) + sortKey->abbreviate = (i == 0 && base->haveDatum1); Assert(sortKey->ssup_attno != 0); @@ -499,7 +504,8 @@ tuplesort_begin_index_gist(Relation heapRel, sortKey->ssup_nulls_first = false; sortKey->ssup_attno = i + 1; /* Convey if abbreviation optimization is applicable in principle */ - sortKey->abbreviate = (i == 0 && base->haveDatum1); + if (sort_abbreviated_keys) + sortKey->abbreviate = (i == 0 && base->haveDatum1); Assert(sortKey->ssup_attno != 0); @@ -573,7 +579,8 @@ tuplesort_begin_datum(Oid datumType, Oid sortOperator, Oid sortCollation, * can't, because a datum sort only stores a single copy of the datum; the * "tuple" field of each SortTuple is NULL. */ - base->sortKeys->abbreviate = !typbyval; + if (sort_abbreviated_keys) + base->sortKeys->abbreviate = !typbyval; PrepareSortSupportFromOrderingOp(sortOperator, base->sortKeys); -- 2.34.1 ^ permalink raw reply [nested|flat] 2+ messages in thread
* [PATCH v8 2/7] Row pattern recognition patch (parse/analysis). @ 2023-09-25 05:01 Tatsuo Ishii <[email protected]> 0 siblings, 0 replies; 2+ messages in thread From: Tatsuo Ishii @ 2023-09-25 05:01 UTC (permalink / raw) --- src/backend/parser/parse_agg.c | 7 + src/backend/parser/parse_clause.c | 293 +++++++++++++++++++++++++++++- src/backend/parser/parse_expr.c | 4 + src/backend/parser/parse_func.c | 3 + 4 files changed, 306 insertions(+), 1 deletion(-) diff --git a/src/backend/parser/parse_agg.c b/src/backend/parser/parse_agg.c index 85cd47b7ae..aa7a1cee80 100644 --- a/src/backend/parser/parse_agg.c +++ b/src/backend/parser/parse_agg.c @@ -564,6 +564,10 @@ check_agglevels_and_constraints(ParseState *pstate, Node *expr) errkind = true; break; + case EXPR_KIND_RPR_DEFINE: + errkind = true; + break; + /* * There is intentionally no default: case here, so that the * compiler will warn if we add a new ParseExprKind without @@ -953,6 +957,9 @@ transformWindowFuncCall(ParseState *pstate, WindowFunc *wfunc, case EXPR_KIND_CYCLE_MARK: errkind = true; break; + case EXPR_KIND_RPR_DEFINE: + errkind = true; + break; /* * There is intentionally no default: case here, so that the diff --git a/src/backend/parser/parse_clause.c b/src/backend/parser/parse_clause.c index 334b9b42bd..293d4b1680 100644 --- a/src/backend/parser/parse_clause.c +++ b/src/backend/parser/parse_clause.c @@ -100,7 +100,10 @@ static WindowClause *findWindowClause(List *wclist, const char *name); static Node *transformFrameOffset(ParseState *pstate, int frameOptions, Oid rangeopfamily, Oid rangeopcintype, Oid *inRangeFunc, Node *clause); - +static void transformRPR(ParseState *pstate, WindowClause *wc, WindowDef *windef, List **targetlist); +static List *transformDefineClause(ParseState *pstate, WindowClause *wc, WindowDef *windef, List **targetlist); +static void transformPatternClause(ParseState *pstate, WindowClause *wc, WindowDef *windef); +static List *transformMeasureClause(ParseState *pstate, WindowClause *wc, WindowDef *windef); /* * transformFromClause - @@ -2950,6 +2953,10 @@ transformWindowDefinitions(ParseState *pstate, rangeopfamily, rangeopcintype, &wc->endInRangeFunc, windef->endOffset); + + /* Process Row Pattern Recognition related clauses */ + transformRPR(pstate, wc, windef, targetlist); + wc->runCondition = NIL; wc->winref = winref; @@ -3815,3 +3822,287 @@ transformFrameOffset(ParseState *pstate, int frameOptions, return node; } + +/* + * transformRPR + * Process Row Pattern Recognition related clauses + */ +static void +transformRPR(ParseState *pstate, WindowClause *wc, WindowDef *windef, List **targetlist) +{ + /* + * Window definition exists? + */ + if (windef == NULL) + return; + + /* + * Row Pattern Common Syntax clause exists? + */ + if (windef->rpCommonSyntax == NULL) + return; + + /* Check Frame option. Frame must start at current row */ + if ((wc->frameOptions & FRAMEOPTION_START_CURRENT_ROW) == 0) + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("FRAME must start at current row when row patttern recognition is used"))); + + /* Transform AFTER MACH SKIP TO clause */ + wc->rpSkipTo = windef->rpCommonSyntax->rpSkipTo; + + /* Transform SEEK or INITIAL clause */ + wc->initial = windef->rpCommonSyntax->initial; + + /* Transform DEFINE clause into list of TargetEntry's */ + wc->defineClause = transformDefineClause(pstate, wc, windef, targetlist); + + /* Check PATTERN clause and copy to patternClause */ + transformPatternClause(pstate, wc, windef); + + /* Transform MEASURE clause */ + transformMeasureClause(pstate, wc, windef); +} + +/* + * transformDefineClause Process DEFINE clause and transform ResTarget into + * list of TargetEntry. + * + * XXX we only support column reference in row pattern definition search + * condition, e.g. "price". <row pattern definition variable name>.<column + * reference> is not supported, e.g. "A.price". + */ +static List * +transformDefineClause(ParseState *pstate, WindowClause *wc, WindowDef *windef, List **targetlist) +{ + /* DEFINE variable name initials */ + static char *defineVariableInitials = "abcdefghijklmnopqrstuvwxyz"; + + ListCell *lc, *l; + ResTarget *restarget, *r; + List *restargets; + char *name; + int initialLen; + int i; + + /* + * If Row Definition Common Syntax exists, DEFINE clause must exist. + * (the raw parser should have already checked it.) + */ + Assert(windef->rpCommonSyntax->rpDefs != NULL); + + /* + * Check and add "A AS A IS TRUE" if pattern variable is missing in DEFINE + * per the SQL standard. + */ + restargets = NIL; + foreach(lc, windef->rpCommonSyntax->rpPatterns) + { + A_Expr *a; + bool found = false; + + if (!IsA(lfirst(lc), A_Expr)) + ereport(ERROR, + errmsg("node type is not A_Expr")); + + a = (A_Expr *)lfirst(lc); + name = strVal(a->lexpr); + + foreach(l, windef->rpCommonSyntax->rpDefs) + { + restarget = (ResTarget *)lfirst(l); + + if (!strcmp(restarget->name, name)) + { + found = true; + break; + } + } + + if (!found) + { + /* + * "name" is missing. So create "name AS name IS TRUE" ResTarget + * node and add it to the temporary list. + */ + A_Const *n; + + restarget = makeNode(ResTarget); + n = makeNode(A_Const); + n->val.boolval.type = T_Boolean; + n->val.boolval.boolval = true; + n->location = -1; + restarget->name = pstrdup(name); + restarget->indirection = NIL; + restarget->val = (Node *)n; + restarget->location = -1; + restargets = lappend((List *)restargets, restarget); + } + } + + if (list_length(restargets) >= 1) + { + /* add missing DEFINEs */ + windef->rpCommonSyntax->rpDefs = list_concat(windef->rpCommonSyntax->rpDefs, + restargets); + list_free(restargets); + } + + /* + * Check for duplicate row pattern definition variables. The standard + * requires that no two row pattern definition variable names shall be + * equivalent. + */ + restargets = NIL; + foreach(lc, windef->rpCommonSyntax->rpDefs) + { + restarget = (ResTarget *)lfirst(lc); + name = restarget->name; + + /* + * Add DEFINE expression (Restarget->val) to the targetlist as a + * TargetEntry if it does not exist yet. Planner will add the column + * ref var node to the outer plan's target list later on. This makes + * DEFINE expression could access the outer tuple while evaluating + * PATTERN. + * + * XXX: adding whole expressions of DEFINE to the plan.targetlist is + * not so good, because it's not necessary to evalute the expression + * in the target list while running the plan. We should extract the + * var nodes only then add them to the plan.targetlist. + */ + findTargetlistEntrySQL99(pstate, (Node *)restarget->val, targetlist, EXPR_KIND_RPR_DEFINE); + + /* + * Make sure that the row pattern definition search condition is a + * boolean expression. + */ + transformWhereClause(pstate, restarget->val, + EXPR_KIND_RPR_DEFINE, "DEFINE"); + + foreach(l, restargets) + { + char *n; + + r = (ResTarget *) lfirst(l); + n = r->name; + + if (!strcmp(n, name)) + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("row pattern definition variable name \"%s\" appears more than once in DEFINE clause", + name), + parser_errposition(pstate, exprLocation((Node *)r)))); + } + restargets = lappend(restargets, restarget); + } + list_free(restargets); + + /* + * Create list of row pattern DEFINE variable name's initial. + * We assign [a-z] to them (up to 26 variable names are allowed). + */ + restargets = NIL; + i = 0; + initialLen = strlen(defineVariableInitials); + + foreach(lc, windef->rpCommonSyntax->rpDefs) + { + char initial[2]; + + restarget = (ResTarget *)lfirst(lc); + name = restarget->name; + + if (i >= initialLen) + { + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("number of row pattern definition variable names exceeds %d", initialLen), + parser_errposition(pstate, exprLocation((Node *)restarget)))); + } + initial[0] = defineVariableInitials[i++]; + initial[1] = '\0'; + wc->defineInitial = lappend(wc->defineInitial, makeString(pstrdup(initial))); + } + + return transformTargetList(pstate, windef->rpCommonSyntax->rpDefs, + EXPR_KIND_RPR_DEFINE); +} + +/* + * transformPatternClause + * Process PATTERN clause and return PATTERN clause in the raw parse tree + */ +static void +transformPatternClause(ParseState *pstate, WindowClause *wc, WindowDef *windef) +{ + ListCell *lc, *l; + + /* + * Row Pattern Common Syntax clause exists? + */ + if (windef->rpCommonSyntax == NULL) + return; + + /* + * Primary row pattern variable names in PATTERN clause must appear in + * DEFINE clause as row pattern definition variable names. + */ + wc->patternVariable = NIL; + wc->patternRegexp = NIL; + foreach(lc, windef->rpCommonSyntax->rpPatterns) + { + A_Expr *a; + char *name; + char *regexp; + bool found = false; + + if (!IsA(lfirst(lc), A_Expr)) + ereport(ERROR, + errmsg("node type is not A_Expr")); + + a = (A_Expr *)lfirst(lc); + name = strVal(a->lexpr); + + foreach(l, windef->rpCommonSyntax->rpDefs) + { + ResTarget *restarget = (ResTarget *)lfirst(l); + + if (!strcmp(restarget->name, name)) + { + found = true; + break; + } + } + + if (!found) + { + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("primary row pattern variable name \"%s\" does not appear in DEFINE clause", + name), + parser_errposition(pstate, exprLocation((Node *)a)))); + } + wc->patternVariable = lappend(wc->patternVariable, makeString(pstrdup(name))); + regexp = strVal(lfirst(list_head(a->name))); + wc->patternRegexp = lappend(wc->patternRegexp, makeString(pstrdup(regexp))); + } +} + +/* + * transformMeasureClause + * Process MEASURE clause + * XXX MEASURE clause is not supported yet + */ +static List * +transformMeasureClause(ParseState *pstate, WindowClause *wc, WindowDef *windef) +{ + if (windef->rowPatternMeasures == NIL) + return NIL; + + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("%s","MEASURE clause is not supported yet"), + parser_errposition(pstate, exprLocation((Node *)windef->rowPatternMeasures)))); + return NIL; +} diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c index 64c582c344..18b58ac263 100644 --- a/src/backend/parser/parse_expr.c +++ b/src/backend/parser/parse_expr.c @@ -557,6 +557,7 @@ transformColumnRef(ParseState *pstate, ColumnRef *cref) case EXPR_KIND_COPY_WHERE: case EXPR_KIND_GENERATED_COLUMN: case EXPR_KIND_CYCLE_MARK: + case EXPR_KIND_RPR_DEFINE: /* okay */ break; @@ -1770,6 +1771,7 @@ transformSubLink(ParseState *pstate, SubLink *sublink) case EXPR_KIND_VALUES: case EXPR_KIND_VALUES_SINGLE: case EXPR_KIND_CYCLE_MARK: + case EXPR_KIND_RPR_DEFINE: /* okay */ break; case EXPR_KIND_CHECK_CONSTRAINT: @@ -3149,6 +3151,8 @@ ParseExprKindName(ParseExprKind exprKind) return "GENERATED AS"; case EXPR_KIND_CYCLE_MARK: return "CYCLE"; + case EXPR_KIND_RPR_DEFINE: + return "DEFINE"; /* * There is intentionally no default: case here, so that the diff --git a/src/backend/parser/parse_func.c b/src/backend/parser/parse_func.c index b3f0b6a137..2ff3699538 100644 --- a/src/backend/parser/parse_func.c +++ b/src/backend/parser/parse_func.c @@ -2656,6 +2656,9 @@ check_srf_call_placement(ParseState *pstate, Node *last_srf, int location) case EXPR_KIND_CYCLE_MARK: errkind = true; break; + case EXPR_KIND_RPR_DEFINE: + errkind = true; + break; /* * There is intentionally no default: case here, so that the -- 2.25.1 ----Next_Part(Mon_Sep_25_14_26_30_2023_752)-- Content-Type: Text/X-Patch; charset=us-ascii Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="v8-0003-Row-pattern-recognition-patch-planner.patch" ^ permalink raw reply [nested|flat] 2+ messages in thread
end of thread, other threads:[~2023-09-25 05:01 UTC | newest] Thread overview: 2+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2023-01-25 21:30 Re: GUCs to control abbreviated sort keys Jeff Davis <[email protected]> 2023-09-25 05:01 [PATCH v8 2/7] Row pattern recognition patch (parse/analysis). Tatsuo Ishii <[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