Re: Row pattern recognition

On Tue, Jul 7, 2026 at 11:40 AM Henson Choi <assam258(at)gmail(dot)com> wrote:
>
> That said, I don't think we should take it into the patch at this
> stage. The current version has one proven defect: a plain
> FIRST(expr, n) with n > 0 loses its forward reach. I added a test for
> it -- "DEFINE A AS v > FIRST(v, 5)" in rpr_explain -- where the
> baseline prints "Nav Mark Lookahead: 5" and the patch prints 0. The
> query result is still correct (the mark is under-advanced, so it only
> keeps extra rows), but the EXPLAIN is wrong and the tuplestore trim is
> lost, so it is a regression against the baseline.
>
> The test will come with the next patch email I send, so you will have
> it there to diff against.
>

The case you mentioned:
PATTERN (A) DEFINE A AS v > FIRST(v, 5)

is fixable easily, it was a mistake I made while refactoring
visit_nav_exec to cut
down the number of eval_nav_offset() calls.

> More broadly, though, reshaping the offset handling this much right as
> we are stabilizing for commit feels like the wrong time -- at this
> point I would rather limit changes to minimal fixes for proven
> defects. This is a worthwhile cleanup that deserves proper review
> time, and it would be more valuable if it can be refined without that
> pressure.
>
> So how about, once the current patch is verified and committed, we
> pursue this as a separate follow-up?

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.

Committing the main patch and then posting a separate "make it readable"
refactoring afterward feels weird to me.
Refactoring now benefits everyone by significantly improving readability.

Broadly, I'd defend patch [2] on the same grounds: it improves readability, and
The current patchset doesn't need code that complex. It may not be the most
future-proof approach (I'm not certain) but given how complicated the whole
patchset already is, making each part more readable is a clear win for
reviewers.

Let's improve readability now while we can. Even assuming the patch is
committable today, there are still 9 months to refine it, so there's no rush
that justifies skipping cleanups. Each refactor makes the code more readable and
easier for reviewers to follow, which benefits everyone.

There won't be unlimited rounds of readability refactoring. Each
refactor makes the code clearer.
---------------------------------------------------
I hope the above is convincing.

[1] https://www.postgresql.org/message-id/CACJufxFfMJAMc0R6g4f7Bwr47QDY%2BOBr2vzqqMRbCzjdD9vR2g%40mail.gmail.com
[2] https://postgr.es/m/CAAAe_zC6iN79qEOgD2TzLHxA8i4T9R3pYtn_5dsQV8FXn3cEcA@mail.gmail.com

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