MIME-Version: 1.0
References: 
 <CAAAe_zBMvzn6ZwmTPhsi0mP6FPVv3hZ2cb1Xe3KD8brT5EPTFg@mail.gmail.com>
 <20260704.162841.39857602849942465.ishii@postgresql.org>
 <CAAAe_zAZCeXAP73q01zdXXc2tTG7=iNM1tX4XgtbTWb02ef1-Q@mail.gmail.com>
 <20260706.150221.1117486145015185971.ishii@postgresql.org>
 <CAAAe_zBtXQPqcRq3vDoOX+1zJ6bYEpJbp4ohFn2AkVC-UoxCKw@mail.gmail.com>
In-Reply-To: 
 <CAAAe_zBtXQPqcRq3vDoOX+1zJ6bYEpJbp4ohFn2AkVC-UoxCKw@mail.gmail.com>
From: jian he <jian.universality@gmail.com>
Date: Thu, 9 Jul 2026 13:55:41 +0800
Message-ID: 
 <CACJufxG9yPZhK9WKee_HEuKo35XkU-eLEhWFUiYAukSAat0G=g@mail.gmail.com>
Subject: Re: Row pattern recognition
To: assam258@gmail.com
Cc: Tatsuo Ishii <ishii@postgresql.org>, pgsql-hackers@postgresql.org,
	zsolt.parragi@percona.com, sjjang112233@gmail.com, vik@postgresfriends.org,
	er@xs4all.nl, jacob.champion@enterprisedb.com, david.g.johnston@gmail.com,
	peter@eisentraut.org, li.evan.chao@gmail.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CACJufxG9yPZhK9WKee_HEuKo35XkU-eLEhWFUiYAukSAat0G%3Dg%40mail.gmail.com>
Precedence: bulk

On Tue, Jul 7, 2026 at 11:40=E2=80=AFAM Henson Choi <assam258@gmail.com> wr=
ote:
>
> 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 mess=
age:
```
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 mu=
ch.
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 o=
ne
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 rus=
h
that justifies skipping cleanups. Each refactor makes the code more readabl=
e 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%2BO=
Br2vzqqMRbCzjdD9vR2g%40mail.gmail.com
[2] https://postgr.es/m/CAAAe_zC6iN79qEOgD2TzLHxA8i4T9R3pYtn_5dsQV8FXn3cEcA=
@mail.gmail.com


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