public inbox for [email protected]help / color / mirror / Atom feed
Re: JIT: The nullness of casetest.value can be determined at the JIT compile time. 5+ messages / 4 participants [nested] [flat]
* Re: JIT: The nullness of casetest.value can be determined at the JIT compile time. @ 2024-09-03 15:18 Xing Guo <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Xing Guo @ 2024-09-03 15:18 UTC (permalink / raw) To: Andreas Karlsson <[email protected]>; +Cc: [email protected] On Tue, Sep 3, 2024 at 8:09 PM Andreas Karlsson <[email protected]> wrote: > > On 8/31/24 10:04 AM, Xing Guo wrote: > > The nullness of casetest.value can be determined at the JIT compile > > time. We can emit fewer codes by utilizing this property. The attached > > patch is trying to fix it. > > I have not reviewed the code yet but the idea seems good. > > But I wonder if we shouldn't instead simplify the code a bit by > specializing these steps when generating them instead of doing the work > runtime/while generating machine code. Yes, I doubt the performance > benefits matter but I personally think the code is cleaner before my > patch than after it. +1 to the idea. > Long term it would be nice to get rid off > caseValue_datum/domainValue_datum as mentioned by Andres[1] but that is > a bigger job so think that either your patch or my patch would make > sense to apply before that. I think your patch makes more sense than mine! Thanks! Best Regards, Xing ^ permalink raw reply [nested|flat] 5+ messages in thread
* [PATCH v23 2/8] Row pattern recognition patch (parse/analysis). @ 2024-10-25 03:56 Tatsuo Ishii <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: Tatsuo Ishii @ 2024-10-25 03:56 UTC (permalink / raw) --- src/backend/parser/parse_agg.c | 7 + src/backend/parser/parse_clause.c | 297 +++++++++++++++++++++++++++++- src/backend/parser/parse_expr.c | 6 + src/backend/parser/parse_func.c | 3 + 4 files changed, 312 insertions(+), 1 deletion(-) diff --git a/src/backend/parser/parse_agg.c b/src/backend/parser/parse_agg.c index efa730c167..a80263f90d 100644 --- a/src/backend/parser/parse_agg.c +++ b/src/backend/parser/parse_agg.c @@ -580,6 +580,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 @@ -970,6 +974,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 4c97690908..a6d2f7f4ba 100644 --- a/src/backend/parser/parse_clause.c +++ b/src/backend/parser/parse_clause.c @@ -98,7 +98,14 @@ static WindowClause *findWindowClause(List *wclist, const char *name); static Node *transformFrameOffset(ParseState *pstate, int frameOptions, Oid rangeopfamily, Oid rangeopcintype, Oid *inRangeFunc, Node *clause); - +static void transformRPR(ParseState *pstate, WindowClause *wc, WindowDef *windef, + List **targetlist); +static List *transformDefineClause(ParseState *pstate, WindowClause *wc, WindowDef *windef, + List **targetlist); +static void transformPatternClause(ParseState *pstate, WindowClause *wc, + WindowDef *windef); +static List *transformMeasureClause(ParseState *pstate, WindowClause *wc, + WindowDef *windef); /* * transformFromClause - @@ -2956,6 +2963,10 @@ transformWindowDefinitions(ParseState *pstate, rangeopfamily, rangeopcintype, &wc->endInRangeFunc, windef->endOffset); + + /* Process Row Pattern Recognition related clauses */ + transformRPR(pstate, wc, windef, targetlist); + wc->winref = winref; result = lappend(result, wc); @@ -3823,3 +3834,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 AFTER MACH SKIP TO variable */ + wc->rpSkipVariable = windef->rpCommonSyntax->rpSkipVariable; + + /* Transform SEEK or INITIAL clause */ + wc->initial = windef->rpCommonSyntax->initial; + + /* Transform DEFINE clause into list of TargetEntry's */ + wc->defineClause = transformDefineClause(pstate, wc, windef, targetlist); + + /* Check PATTERN clause and copy to patternClause */ + transformPatternClause(pstate, wc, windef); + + /* Transform MEASURE clause */ + transformMeasureClause(pstate, wc, windef); +} + +/* + * transformDefineClause + * Process DEFINE clause and transform ResTarget into list of + * TargetEntry. + * + * XXX we only support column reference in row pattern definition search + * condition, e.g. "price". <row pattern definition variable name>.<column + * reference> is not supported, e.g. "A.price". + */ +static List * +transformDefineClause(ParseState *pstate, WindowClause *wc, WindowDef *windef, + List **targetlist) +{ + /* DEFINE variable name initials */ + static char *defineVariableInitials = "abcdefghijklmnopqrstuvwxyz"; + + ListCell *lc, + *l; + ResTarget *restarget, + *r; + List *restargets; + List *defineClause; + char *name; + int initialLen; + int i; + + /* + * If Row Definition Common Syntax exists, DEFINE clause must exist. (the + * raw parser should have already checked it.) + */ + Assert(windef->rpCommonSyntax->rpDefs != NULL); + + /* + * Check and add "A AS A IS TRUE" if pattern variable is missing in DEFINE + * per the SQL standard. + */ + restargets = NIL; + foreach(lc, windef->rpCommonSyntax->rpPatterns) + { + A_Expr *a; + bool found = false; + + if (!IsA(lfirst(lc), A_Expr)) + ereport(ERROR, + errmsg("node type is not A_Expr")); + + a = (A_Expr *) lfirst(lc); + name = strVal(a->lexpr); + + foreach(l, windef->rpCommonSyntax->rpDefs) + { + restarget = (ResTarget *) lfirst(l); + + if (!strcmp(restarget->name, name)) + { + found = true; + break; + } + } + + if (!found) + { + /* + * "name" is missing. So create "name AS name IS TRUE" ResTarget + * node and add it to the temporary list. + */ + A_Const *n; + + restarget = makeNode(ResTarget); + n = makeNode(A_Const); + n->val.boolval.type = T_Boolean; + n->val.boolval.boolval = true; + n->location = -1; + restarget->name = pstrdup(name); + restarget->indirection = NIL; + restarget->val = (Node *) n; + restarget->location = -1; + restargets = lappend((List *) restargets, restarget); + } + } + + if (list_length(restargets) >= 1) + { + /* add missing DEFINEs */ + windef->rpCommonSyntax->rpDefs = + list_concat(windef->rpCommonSyntax->rpDefs, restargets); + list_free(restargets); + } + + /* + * Check for duplicate row pattern definition variables. The standard + * requires that no two row pattern definition variable names shall be + * equivalent. + */ + restargets = NIL; + foreach(lc, windef->rpCommonSyntax->rpDefs) + { + restarget = (ResTarget *) lfirst(lc); + name = restarget->name; + + /* + * Add DEFINE expression (Restarget->val) to the targetlist as a + * TargetEntry if it does not exist yet. Planner will add the column + * ref var node to the outer plan's target list later on. This makes + * DEFINE expression could access the outer tuple while evaluating + * PATTERN. + * + * XXX: adding whole expressions of DEFINE to the plan.targetlist is + * not so good, because it's not necessary to evalute the expression + * in the target list while running the plan. We should extract the + * var nodes only then add them to the plan.targetlist. + */ + findTargetlistEntrySQL99(pstate, (Node *) restarget->val, + targetlist, EXPR_KIND_RPR_DEFINE); + + /* + * Make sure that the row pattern definition search condition is a + * boolean expression. + */ + transformWhereClause(pstate, restarget->val, + EXPR_KIND_RPR_DEFINE, "DEFINE"); + + foreach(l, restargets) + { + char *n; + + r = (ResTarget *) lfirst(l); + n = r->name; + + if (!strcmp(n, name)) + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("row pattern definition variable name \"%s\" appears more than once in DEFINE clause", + name), + parser_errposition(pstate, exprLocation((Node *) r)))); + } + restargets = lappend(restargets, restarget); + } + list_free(restargets); + + /* + * Create list of row pattern DEFINE variable name's initial. We assign + * [a-z] to them (up to 26 variable names are allowed). + */ + restargets = NIL; + i = 0; + initialLen = strlen(defineVariableInitials); + + foreach(lc, windef->rpCommonSyntax->rpDefs) + { + char initial[2]; + + restarget = (ResTarget *) lfirst(lc); + name = restarget->name; + + if (i >= initialLen) + { + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("number of row pattern definition variable names exceeds %d", + initialLen), + parser_errposition(pstate, + exprLocation((Node *) restarget)))); + } + initial[0] = defineVariableInitials[i++]; + initial[1] = '\0'; + wc->defineInitial = lappend(wc->defineInitial, + makeString(pstrdup(initial))); + } + + defineClause = transformTargetList(pstate, windef->rpCommonSyntax->rpDefs, + EXPR_KIND_RPR_DEFINE); + + /* mark column origins */ + markTargetListOrigins(pstate, defineClause); + + /* mark all nodes in the DEFINE clause tree with collation information */ + assign_expr_collations(pstate, (Node *) defineClause); + + return defineClause; +} + +/* + * transformPatternClause + * Process PATTERN clause and return PATTERN clause in the raw parse tree + */ +static void +transformPatternClause(ParseState *pstate, WindowClause *wc, + WindowDef *windef) +{ + ListCell *lc; + + /* + * Row Pattern Common Syntax clause exists? + */ + if (windef->rpCommonSyntax == NULL) + return; + + wc->patternVariable = NIL; + wc->patternRegexp = NIL; + foreach(lc, windef->rpCommonSyntax->rpPatterns) + { + A_Expr *a; + char *name; + char *regexp; + + if (!IsA(lfirst(lc), A_Expr)) + ereport(ERROR, + errmsg("node type is not A_Expr")); + + a = (A_Expr *) lfirst(lc); + name = strVal(a->lexpr); + + wc->patternVariable = lappend(wc->patternVariable, makeString(pstrdup(name))); + regexp = strVal(lfirst(list_head(a->name))); + + wc->patternRegexp = lappend(wc->patternRegexp, makeString(pstrdup(regexp))); + } +} + +/* + * transformMeasureClause + * Process MEASURE clause + * XXX MEASURE clause is not supported yet + */ +static List * +transformMeasureClause(ParseState *pstate, WindowClause *wc, + WindowDef *windef) +{ + if (windef->rowPatternMeasures == NIL) + return NIL; + + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("%s", "MEASURE clause is not supported yet"), + parser_errposition(pstate, exprLocation((Node *) windef->rowPatternMeasures)))); + return NIL; +} diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c index ef0b560f5e..b30b9f2675 100644 --- a/src/backend/parser/parse_expr.c +++ b/src/backend/parser/parse_expr.c @@ -577,6 +577,7 @@ transformColumnRef(ParseState *pstate, ColumnRef *cref) case EXPR_KIND_COPY_WHERE: case EXPR_KIND_GENERATED_COLUMN: case EXPR_KIND_CYCLE_MARK: + case EXPR_KIND_RPR_DEFINE: /* okay */ break; @@ -1860,6 +1861,9 @@ transformSubLink(ParseState *pstate, SubLink *sublink) case EXPR_KIND_GENERATED_COLUMN: err = _("cannot use subquery in column generation expression"); break; + case EXPR_KIND_RPR_DEFINE: + err = _("cannot use subquery in DEFINE expression"); + break; /* * There is intentionally no default: case here, so that the @@ -3199,6 +3203,8 @@ ParseExprKindName(ParseExprKind exprKind) return "GENERATED AS"; case EXPR_KIND_CYCLE_MARK: return "CYCLE"; + case EXPR_KIND_RPR_DEFINE: + return "DEFINE"; /* * There is intentionally no default: case here, so that the diff --git a/src/backend/parser/parse_func.c b/src/backend/parser/parse_func.c index 9b23344a3b..4c482abb30 100644 --- a/src/backend/parser/parse_func.c +++ b/src/backend/parser/parse_func.c @@ -2658,6 +2658,9 @@ check_srf_call_placement(ParseState *pstate, Node *last_srf, int location) case EXPR_KIND_CYCLE_MARK: errkind = true; break; + case EXPR_KIND_RPR_DEFINE: + errkind = true; + break; /* * There is intentionally no default: case here, so that the -- 2.25.1 ----Next_Part(Fri_Oct_25_13_04_53_2024_648)-- Content-Type: Text/X-Patch; charset=us-ascii Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="v23-0003-Row-pattern-recognition-patch-rewriter.patch" ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: JIT: The nullness of casetest.value can be determined at the JIT compile time. @ 2024-11-15 09:38 Heikki Linnakangas <[email protected]> parent: Xing Guo <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Heikki Linnakangas @ 2024-11-15 09:38 UTC (permalink / raw) To: Xing Guo <[email protected]>; Andreas Karlsson <[email protected]>; +Cc: [email protected] On 03/09/2024 18:18, Xing Guo wrote: > On Tue, Sep 3, 2024 at 8:09 PM Andreas Karlsson <[email protected]> wrote: >> >> On 8/31/24 10:04 AM, Xing Guo wrote: >>> The nullness of casetest.value can be determined at the JIT compile >>> time. We can emit fewer codes by utilizing this property. The attached >>> patch is trying to fix it. >> >> I have not reviewed the code yet but the idea seems good. >> >> But I wonder if we shouldn't instead simplify the code a bit by >> specializing these steps when generating them instead of doing the work >> runtime/while generating machine code. Yes, I doubt the performance >> benefits matter but I personally think the code is cleaner before my >> patch than after it. > > +1 to the idea. > >> Long term it would be nice to get rid off >> caseValue_datum/domainValue_datum as mentioned by Andres[1] but that is >> a bigger job so think that either your patch or my patch would make >> sense to apply before that. > > I think your patch makes more sense than mine! Thanks! Huge +1 for cleaning up this abuse of caseValue_datum/domainValue_datum. While correct and sensible if we continue the abuse, these patches feel like putting lipstick on a pig. I think the most straightforward way to clean that up is to replace caseValue and domainValue in ExprContext with a generic array of "expression params", and a new Expr node type to refer to them. Basically, the same as we have now, but with explicit names to indicate that the values are supplied externally. In https://www.postgresql.org/message-id/20230302200549.l2ikytmnqzvy5a7a%40alap3.anarazel.de, Andres hinted at using ParamExecData for these. That'd be nice, but seems harder. Those params are reserved at planning time, so we'd need to somehow allocate more of them in ExecInitExpr, or move the bookkeeping to the planner. But just basically renaming "caseValue" to "externalValue" seems straightforward. PARAM_EXECs are also a slightly more expensive to evaluate, because they support lazy evaluation of the subplan. This goes off-topic, but I wonder if there are cases where we could know at ExecInitExpr() time that the parameter must be already evaluated when it's referred, and skip the lazy checks? -- Heikki Linnakangas Neon (https://neon.tech) ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: JIT: The nullness of casetest.value can be determined at the JIT compile time. @ 2025-01-30 03:29 Tom Lane <[email protected]> parent: Heikki Linnakangas <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Tom Lane @ 2025-01-30 03:29 UTC (permalink / raw) To: Heikki Linnakangas <[email protected]>; +Cc: Xing Guo <[email protected]>; Andreas Karlsson <[email protected]>; [email protected] Heikki Linnakangas <[email protected]> writes: > Huge +1 for cleaning up this abuse of caseValue_datum/domainValue_datum. > While correct and sensible if we continue the abuse, these patches feel > like putting lipstick on a pig. Agreed. I spent some time trying to do it better, with results shown at [1]. If we adopt that idea, then the executor's support for CaseTestExpr will go away, so there's little point in pursuing that half of the patch given here. However, I concluded that there's insufficient reason to redesign CoerceToDomainValue, so we could still push forward with that half of this patch. regards, tom lane [1] https://www.postgresql.org/message-id/[email protected] ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: JIT: The nullness of casetest.value can be determined at the JIT compile time. @ 2025-01-30 18:27 Tom Lane <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: Tom Lane @ 2025-01-30 18:27 UTC (permalink / raw) To: Heikki Linnakangas <[email protected]>; +Cc: Xing Guo <[email protected]>; Andreas Karlsson <[email protected]>; [email protected] I wrote: > Agreed. I spent some time trying to do it better, with results > shown at [1]. If we adopt that idea, then the executor's support > for CaseTestExpr will go away, so there's little point in pursuing > that half of the patch given here. However, I concluded that there's > insufficient reason to redesign CoerceToDomainValue, so we could > still push forward with that half of this patch. On further consideration, we might as well do this whole patch. The patch I'm proposing over there is large and might well not land in v18 (or ever), so let's get this low-hanging fruit. I pushed Andreas's patch with some cosmetic tidying, mainly that I felt it better not to try to combine duplicate-ish code for the EEOP_CASE_TESTVAL and EEOP_DOMAIN_TESTVAL cases. We're after understandability here after all, and it's still a net code savings even with that duplication. regards, tom lane ^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2025-01-30 18:27 UTC | newest] Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2024-09-03 15:18 Re: JIT: The nullness of casetest.value can be determined at the JIT compile time. Xing Guo <[email protected]> 2024-11-15 09:38 ` Heikki Linnakangas <[email protected]> 2025-01-30 03:29 ` Tom Lane <[email protected]> 2025-01-30 18:27 ` Tom Lane <[email protected]> 2024-10-25 03:56 [PATCH v23 2/8] 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