On Tue, Jul 7, 2026 at 11:40 AM Henson Choi <[email protected]> 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_5dsqv8fxn3c...@mail.gmail.com -- jian https://www.enterprisedb.com/
