On Thu, Jul 9, 2026 at 1:55 PM jian he <[email protected]> wrote: > > > That refactor is the patch I posted in [1]. An excerpt from its commit > message: > ``` > Row pattern navigation (PREV/NEXT/FIRST/LAST and their compounds) resolves > offsets (transforming an expression into an int64) in ExecInitExprRec, > ExecEvalRPRNavSet, extract_const_offset and eval_define_offsets. Several > places transform the same offset expression into an int64 -- that is too much. > With the attached, a single place (ExecInitWindowAgg -> eval_define_offsets) > handles this. > ``` > > I think this belongs in the patchset now, rather than as a follow-up after the > main patch is committed. Computing the same offset in several places hurts > readability and, as this bug shows, is error-prone; consolidating it into one > place makes the code easier to read for future reviewers. >
Hi. Still based on https://github.com/assam258-5892/postgres/commits/RPR Please check the attached v51-0001, v51-0002, v51-0003. v51-0001 fixes the regression failure v51-0002: If the target row does not exist, skip evaluating the navigation function's argument expression v51-0003: Introduce execution struct RprNavState For example: CREATE TABLE t (id int, v int); INSERT INTO t VALUES (1, 10); SELECT id, count(*) OVER w AS cnt FROM t WINDOW w AS (ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING PATTERN (A) DEFINE A AS PREV(v IS NOT NULL) IS NULL); Here the first row has no PREV row, so the argument expression (v IS NOT NULL) must not be evaluated at all; PREV(...) itself returns NULL, and PREV(...) IS NULL is therefore true. The expected cnt value is 1. To implement this, an extra step EEOP_JUMP_IF_NULL is emitted right after EEOP_RPR_NAV_SET, jumping over the argument expression's steps directly to EEOP_RPR_NAV_RESTORE in case of row does not exists. ------------------------------------------------------------------------ Storing the resolved offset values (offset, compound_offset) on RPRNavExpr, next to the offset expressions they are computed from (offset_arg, compound_offset_arg), seems wrong to me: the expressions are plan-tree fields, but the resolved values are per-execution results, and the executor must treat the plan tree as read-only. Compare how FOR PORTION OF handles this in ExecInitModifyTable: ``` exprState = ExecPrepareExpr((Expr *) forPortionOf->targetRange, estate); targetRange = ExecEvalExpr(exprState, econtext, &isNull); /* Create state for FOR PORTION OF operation */ fpoState = makeNode(ForPortionOfState); fpoState->fp_targetRange = targetRange; ``` The expression is evaluated at executor startup, and the resulting value is stored in an executor-state struct — the plan node is never modified. We should do the same here: evaluate the offset expressions at executor startup and store the resulting values in executor state. So in v51-0003 I added a new executor-state struct, RprNavState, which captures everything the evaluation code in execExprInterp.c needs: the WindowAggState, the RPRNavExpr, the resolved offset and compound_offset, and the result type info (resulttyplen, resulttypbyval). Keeping such per-node evaluation state in an out-of-line struct, with the ExprEvalStep holding just a pointer to it, follows an existing convention in the expression evaluation code — see JsonExprState for a similar example. -- jian https://www.enterprisedb.com/
v51-0003-Introduce-execution-struct-RprNavState.rpr
Description: Binary data
v51-0001-Fix-the-regression-failure.rpr
Description: Binary data
v51-0002-Stop-evaluating-navigation-arguments-in-case-rows-not-exists.rpr
Description: Binary data
