public inbox for [email protected]
help / color / mirror / Atom feedFrom: Chengpeng Yan <[email protected]>
To: John Naylor <[email protected]>
Cc: David Rowley <[email protected]>
Cc: Richard Guo <[email protected]>
Cc: [email protected] <[email protected]>
Cc: [email protected] <[email protected]>
Subject: Re: BUG #19533: Wrong results from WindowAgg run-condition pushdown on count() with EXCLUDE CURRENT ROW
Date: Sun, 5 Jul 2026 12:54:38 +0000
Message-ID: <[email protected]> (raw)
In-Reply-To: <CANWCAZap+k7-E2f-XoC36Hk4izZjbN8w2c9OuomummGfZ5d20w@mail.gmail.com>
References: <[email protected]>
<[email protected]>
<CAMbWs4-6umOecRifmC88RW0SHWDAvb4c3-VD8XQ3NJufsUyUOQ@mail.gmail.com>
<CAApHDvqHXZpF9pPStLqPp5O=G2gVpPD959bMTRRkLjdy4Tvx7g@mail.gmail.com>
<CAApHDvoJ07GM6V2QWyJAgsJcM-dcFS+nwPq4Um=-NsXv5PmoCw@mail.gmail.com>
<CANWCAZbiYQ+=CHAaftwp2x83Gy6tJCB8GUe15yO5DW-cpp0Mxg@mail.gmail.com>
<CAApHDvrN-HL=dSBez=pme5RQKCiH7SrsMEFV-eQRJN3Vs94ZEQ@mail.gmail.com>
<CANWCAZap+k7-E2f-XoC36Hk4izZjbN8w2c9OuomummGfZ5d20w@mail.gmail.com>
Hi,
> On Jul 5, 2026, at 18:44, John Naylor <[email protected]> wrote:
>
> I was going to share the three examples, but then I had a better idea:
> I had Claude generate a PL/pgSQL script to exhaustively (or something
> close to it) iterate through the the whole space on a few different
> data set shapes, look for plans with run conditions, and compare the
> result to the "c+0" oracle. (attached as
> count_runcondition_matrix.sql). On master, it found 295 (!) distinct
> window specs that give wrong answers with run conditions on at least
> one data set (attached as runcond-failures-master.txt). With v2, that
> number falls to 162, which is still a surprisingly large number
> (attached as runcond-failures-v2.txt). This script takes a while to
> run, so it's probably not a good basis for a regression test, but it
> seems useful to test against, and it can be modified to test
> hypotheses.
>
> I then had Claude generate two patches against master, attached as v3.
> One is the minimal fix to get the above test matrix to pass. I didn't
> try to determine how many currently working cases got lost that way.
> The other is an attempt to provide complete coverage of all
> monotonicity proofs. The latter is quite an indigestible mess, so we
> probably cannot commit something like that, but I think a reasonable
> takeaway is that EXCLUDE presents a really difficult set of cases.
>
> I'll also note that two of v2's tests are valid on the data they work
> on, but are not valid in general. I've attached v2-wrong-test-repo.sql
> to demonstrate.
Thanks for putting together the matrix script. That seems like a very
good way to test hypotheses here.
I have also been thinking about this tradeoff while trying to put
together a patch for it. It looks possible to prove more EXCLUDE cases,
but the code and the test space seem to get complicated quickly. My
current inclination is to be conservative here: return
`MONOTONICFUNC_NONE` unless a case is clearly proven safe. The matrix
results seem to support that direction. That may miss some
run-condition opportunities, but it avoids relying on a monotonicity
claim that is not yet fully justified.
This is also close to why the v1 patch I posted earlier simply disabled
the EXCLUDE cases. That version missed the no-ORDER-BY issue, but the
conservative part matches the direction of your minimal v3: do not try
to infer monotonicity for excluded frames unless the case is very
simple.
One extra axis I noticed in the matrix is FILTER. It currently
enumerates `count(*)` and `count(e)`, but not `count(...) FILTER (...)`,
for example:
```
count(*) FILTER (WHERE ...)
```
That is not the same as plain `count(*)`, and it makes the set of
combinations even larger.
I was about to send a WIP patch that is very close to your minimal v3.
The main code difference is this guard:
```
int exclusion = frameOptions & FRAMEOPTION_EXCLUSION;
bool plain_count_star = req->window_func->winfnoid == F_COUNT_ &&
req->window_func->aggfilter == NULL;
if (exclusion &&
(exclusion != FRAMEOPTION_EXCLUDE_CURRENT_ROW || !plain_count_star))
{
req->monotonic = MONOTONICFUNC_NONE;
PG_RETURN_POINTER(req);
}
```
So compared with the minimal v3 rule that rejects all EXCLUDE cases,
this only keeps plain `count(*) EXCLUDE CURRENT ROW`, with no FILTER.
The reason is that removing the current row removes a fixed
contribution. Everything else with EXCLUDE returns
`MONOTONICFUNC_NONE`. I am not strongly attached to keeping that case
if people prefer the simpler minimal fix, but I think the general shape
should still be reject-by-default rather than allow-by-default.
So I agree with the takeaway from your complete-coverage experiment:
there are valid optimizations in this area, but proving all of them
seems too complex for a bug fix. Starting from the minimal safe rule
and adding back only small, clearly proven cases seems safer to me.
--
Best regards,
Chengpeng Yan
view thread (15+ 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]
Subject: Re: BUG #19533: Wrong results from WindowAgg run-condition pushdown on count() with EXCLUDE CURRENT ROW
In-Reply-To: <[email protected]>
* 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