Re: Bug in row_number() optimization

2022-12-04 Thread David Rowley
On Thu, 1 Dec 2022 at 21:18, Richard Guo wrote: >> + if (!func_strict(opexpr->opfuncid)) >> + return false; >> >> Should return true instead? > > > Yeah, you're right. This should be a thinko. Yeah, oops. That's wrong. I've adjusted that in the attached. I'm keen to move along and

Re: Bug in row_number() optimization

2022-12-04 Thread David Rowley
On Mon, 28 Nov 2022 at 22:59, Sergey Shinderuk wrote: > > On 28.11.2022 03:23, David Rowley wrote: > > On Sat, 26 Nov 2022 at 05:19, Tom Lane wrote: > >> It's pretty unlikely that this would work during an actual index scan. > >> I'm fairly sure that btree (and other index AMs) have hard-wired >

Re: Bug in row_number() optimization

2022-12-04 Thread David Rowley
On Fri, 2 Dec 2022 at 00:21, Sergey Shinderuk wrote: > Maybe I'm missing something, but the previous call to spool_tuples() > might have read extra tuples (if the tuplestore spilled to disk), and > after switching to WINDOWAGG_PASSTHROUGH_STRICT mode we nevertheless > would loop through these

Re: Bug in row_number() optimization

2022-12-01 Thread Sergey Shinderuk
On 01.12.2022 11:18, Richard Guo wrote: On Mon, Nov 28, 2022 at 5:59 PM Sergey Shinderuk mailto:s.shinde...@postgrespro.ru>> wrote: Not quite sure that we don't need to do anything for the WINDOWAGG_PASSTHROUGH_STRICT case. Although, we won't return any more tuples for the

Re: Bug in row_number() optimization

2022-12-01 Thread Richard Guo
On Mon, Nov 28, 2022 at 5:59 PM Sergey Shinderuk wrote: > Not quite sure that we don't need to do anything for the > WINDOWAGG_PASSTHROUGH_STRICT case. Although, we won't return any more > tuples for the current partition, we still call ExecProject with > dangling pointers. Is it okay? AFAIU

Re: Bug in row_number() optimization

2022-11-28 Thread Sergey Shinderuk
On 28.11.2022 03:23, David Rowley wrote: On Sat, 26 Nov 2022 at 05:19, Tom Lane wrote: Sergey Shinderuk writes: What about user-defined operators? I created my own <= operator for int8 which returns true on null input, and put it in a btree operator class. Admittedly, it's weird that (null

Re: Bug in row_number() optimization

2022-11-27 Thread David Rowley
On Sat, 26 Nov 2022 at 05:19, Tom Lane wrote: > > Sergey Shinderuk writes: > > What about user-defined operators? I created my own <= operator for int8 > > which returns true on null input, and put it in a btree operator class. > > Admittedly, it's weird that (null <= 1) evaluates to true. But

Re: Bug in row_number() optimization

2022-11-25 Thread Tom Lane
Sergey Shinderuk writes: > What about user-defined operators? I created my own <= operator for int8 > which returns true on null input, and put it in a btree operator class. > Admittedly, it's weird that (null <= 1) evaluates to true. But does it > violate the contract of the btree operator

Re: Bug in row_number() optimization

2022-11-25 Thread Sergey Shinderuk
On 25.11.2022 15:46, Richard Guo wrote: Considering the 'Filter' is a strict function, marking it as NULL would do.  I think this is why this patch works. What about user-defined operators? I created my own <= operator for int8 which returns true on null input, and put it in a btree operator

Re: Bug in row_number() optimization

2022-11-25 Thread Richard Guo
On Fri, Nov 25, 2022 at 11:26 AM David Rowley wrote: > There are two different pass-through modes that the WindowAgg can move > into when it detects that the run condition is no longer true: > > 1) WINDOWAGG_PASSTHROUGH and > 2) WINDOWAGG_PASSTHROUGH_STRICT > > #2 is used when the WindowAgg is

Re: Bug in row_number() optimization

2022-11-24 Thread David Rowley
On Fri, 25 Nov 2022 at 16:00, Richard Guo wrote: > Verified the problem is fixed with this patch. I'm not familiar with > the WindowAgg execution codes. As far as I understand, this patch works > because we set ecxt_aggnulls to true, making it a NULL value. And the > top-level WindowAgg node's

Re: Bug in row_number() optimization

2022-11-24 Thread Richard Guo
On Fri, Nov 25, 2022 at 7:34 AM David Rowley wrote: > Since upper-level WindowAggs cannot reference values calculated in > some lower-level WindowAgg, why can't we just NULLify the pointers > instead? See attached. Verified the problem is fixed with this patch. I'm not familiar with the

Re: Bug in row_number() optimization

2022-11-24 Thread David Rowley
On Fri, 25 Nov 2022 at 00:52, Sergey Shinderuk wrote: > Shouldn't we handle any pass-by-reference type the same? I suppose, a > user-defined window function can return some other type, not int8. Thanks for reporting this and to you and Richard for working on a fix. I've just looked at it and it

Re: Bug in row_number() optimization

2022-11-24 Thread Sergey Shinderuk
On 24.11.2022 06:16, Richard Guo wrote: Regarding how to fix this problem, firstly I believe we need to evaluate window functions in the per-tuple memory context, as the HEAD does. When we decide we need to go into pass-through mode, I'm thinking that we can just copy out the results of the last

Re: Bug in row_number() optimization

2022-11-23 Thread Richard Guo
On Tue, Nov 22, 2022 at 3:44 PM Richard Guo wrote: > On Wed, Nov 16, 2022 at 7:38 AM Sergey Shinderuk < > s.shinde...@postgrespro.ru> wrote: > >> The failing query is: >> SELECT * FROM >>(SELECT *, >>count(salary) OVER (PARTITION BY depname || '') c1, -- w1 >>

Re: Bug in row_number() optimization

2022-11-21 Thread Richard Guo
On Wed, Nov 16, 2022 at 7:38 AM Sergey Shinderuk wrote: > The failing query is: > SELECT * FROM >(SELECT *, >count(salary) OVER (PARTITION BY depname || '') c1, -- w1 >row_number() OVER (PARTITION BY depname) rn, -- w2 >count(*) OVER (PARTITION BY depname)

Bug in row_number() optimization

2022-11-15 Thread Sergey Shinderuk
Hello, We've accidentally found a subtle bug introduced by commit 9d9c02ccd1aea8e9131d8f4edb21bf1687e40782 Author: David Rowley Date: Fri Apr 8 10:34:36 2022 +1200 Teach planner and executor about monotonic window funcs On a 32-bit system Valgrind reports use-after-free when running