Re: Row pattern recognition

Hi.

In src/include/nodes/execnodes.h, we're adding quite a few fields to
WindowAggState that are only used for RPR queries.
Should we consolidate these fields behind a single pointer (named
RPRContext) to keep the WindowAggState size smaller for non-RPR
queries?

In function get_reduced_frame_status, I made the following changes:
``````
bool conditionA;
bool conditionB;
bool conditionC;
bool conditionD;
if (!winstate->rpr_match_valid)
return RF_NOT_DETERMINED;
conditionA = (pos == start && winstate->rpr_match_matched && length == 0);
conditionB = (pos < start || pos >= start + length);
conditionC = !winstate->rpr_match_matched;
conditionD = (pos == start);
if ((conditionA && conditionB) || (conditionA && conditionC) ||
(conditionA && conditionD))
{
if (conditionA)
elog(INFO, "conditionA is true");
if (conditionB)
elog(INFO, "conditionB is true");
if (conditionC)
elog(INFO, "conditionC is true");
if (conditionD)
elog(INFO, "conditionD is true");
}
if ((conditionB && conditionC) || (conditionB && conditionD) ||
(conditionC && conditionD))
{
elog(INFO, "more than 2 branch is true, should not be reached");
if (!(conditionC && conditionD))
elog(INFO, "not (conditionCD is true)");
if ((conditionB && conditionD))
elog(INFO, "conditionB and conditionD is true)");
}
``````
I re-ran the regression tests and noticed it's entirely possible for multiple
conditions to be true at once. Because of this, all the IF blocks in
get_reduced_frame_status (after the initial one) are order-dependent. If I move
one IF statement above another, get_reduced_frame_status may return a
different result.
Is this the expected behavior? It seems like a potential logic flaw.

------------------------------------------------------------------------------------
VIII-3. Absorption Conditions

Planner-time prerequisites (all must hold for absorption to be enabled):

(a) SKIP PAST LAST ROW. SKIP TO NEXT ROW creates overlapping
contexts that cannot be safely absorbed.
(b) Unbounded frame (ROWS BETWEEN CURRENT ROW AND UNBOUNDED
FOLLOWING). Limited frames apply differently to each context,
breaking the monotonicity principle.
(c) No match_start-dependent navigation in DEFINE.

Mechanism: each context has a different matchStartRow, so FIRST
resolves to a different row for each context at the same
currentpos. An earlier context's DEFINE result no longer
subsumes a later one's, making count-dominance comparison
invalid. Rather than comparing matchStartRow at runtime
(which would complicate the absorb path), any match_start
dependency disables absorption entirely.

Navigation content match_start dep. absorption
------------------------------------------------------------
No navigation none safe
PREV/NEXT only none safe
LAST (no offset) none safe
LAST (with offset) boundary check unsafe
FIRST (any) direct unsafe
Compound (inner FIRST) direct unsafe
Compound (inner LAST, no off.) none safe
Compound (inner LAST, w/off.) boundary chk unsafe
------------------------------------------------------------------------------------
"boundary chk" should be "boundary check".
column "match_start dep.", I don't understand the meaning of "direct"
and "boundary check".
"count-dominance" seems like an invented word, but I could not find
the explanation.
"match_start-dependent" occurred many times, it would also deserve an
explanation.
We can also rename it as match_start_dependent.

----------------------------------------------------
Attached is a minor refactoring of ExecRPRProcessRow, no need for
boolean hasLimitedFrame.
``if (hasLimitedFrame && winstate->endOffsetValue != 0)`` seems wrong
to me, endOffsetValue is a Datum,
and you directly compare it with 0.
see also calculate_frame_offsets.
----------------------------------------------------
ExecRPRProcessRow,
nfa_evaluate_row
```
if (!window_gettupleslot(winobj, pos, slot))
return false; /* No row exists */
```
indicates that it's not possible for (currentPos > ctxFrameEnd).
therefore
`````
if (currentPos >= ctxFrameEnd)
{
/* Frame boundary exceeded: force mismatch */
nfa_match(winstate, ctx, NULL);
continue;
}
`````
should change to ```if (currentPos == ctxFrameEnd)```.
With that, the comment wording "exceeded" seems wrong too.

The comment 'Frame boundary exceeded' also needs updating since
it's no longer exceeding the boundary.
----------------------------------------------------
Since I couldn't generate a coverage report, I am using elog(INFO) to test
reachability. Rerun the regress tests, you will find out that the ELSE IF branch
below is not reached by current regress tests.
``````
else if (targetCtx->states == NULL)
{
/* Context already completed - skip to result registration */
goto register_result;
elog(INFO, "XXXX reached");
}
``````

--
jian
https://www.enterprisedb.com/