public inbox for [email protected]  

help / color / mirror / Atom feed

From: Henson Choi <[email protected]>
To: [email protected]
To: Tatsuo Ishii <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Subject: Re: Row pattern recognition
Date: Tue, 7 Jul 2026 12:40:24 +0900
Message-ID: <CAAAe_zBtXQPqcRq3vDoOX+1zJ6bYEpJbp4ohFn2AkVC-UoxCKw@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <CAAAe_zBMvzn6ZwmTPhsi0mP6FPVv3hZ2cb1Xe3KD8brT5EPTFg@mail.gmail.com>
	<[email protected]>
	<CAAAe_zAZCeXAP73q01zdXXc2tTG7=iNM1tX4XgtbTWb02ef1-Q@mail.gmail.com>
	<[email protected]>

Hi Jian,

Thanks for this -- consolidating the offset resolution into one place
is a direction I like, and a couple of things about it are genuinely
nice:

> EXPLAIN reads the resolved offsets from the planstate
> (ExecInitWindowAgg is reached for EXPLAIN without ANALYZE), so it now
> prints concrete offsets instead of "runtime".

That is a real improvement -- the offset becomes visible in the plan
even when it comes from a bind parameter, where today we can only show
"runtime".

It also lines up with a point Tatsuo made back in April: since the
planner already folds a constant offset expression down to a single
Const, doing the resolution in the executor means we no longer have to
care whether the offset was constant or not.  So the direction has his
backing as well.

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.

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?

Best regards,
Henson

view thread (181+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: Row pattern recognition
  In-Reply-To: <CAAAe_zBtXQPqcRq3vDoOX+1zJ6bYEpJbp4ohFn2AkVC-UoxCKw@mail.gmail.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox