Re: [HACKERS] Re: Request for Patch Feedback: Lag & Lead Window Functions Can Ignore Nulls

[Dean Rasheed]

On 30 May 2016 at 15:44, Andrew Gierth  wrote:
>> "Dean" == Dean Rasheed  writes:
>
>  Dean> That may be so, but we already support FILTER for all windows
>  Dean> functions as well as aggregates:
>
> Not so:
>
> "If FILTER is specified, then only the input rows for which the
> filter_clause evaluates to true are fed to the window function; other
> rows are discarded. Only window functions that are aggregates accept a
> FILTER clause."
>

Ah, yes. It's all coming back to me now.

Regards,
Dean


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Re: Request for Patch Feedback: Lag & Lead Window Functions Can Ignore Nulls

[Andrew Gierth]

> "Dean" == Dean Rasheed  writes:

 Dean> That may be so, but we already support FILTER for all windows
 Dean> functions as well as aggregates:

Not so:

"If FILTER is specified, then only the input rows for which the
filter_clause evaluates to true are fed to the window function; other
rows are discarded. Only window functions that are aggregates accept a
FILTER clause."

(Per spec, FILTER appears only in the  production,
which is just one of the several options for .)

-- 
Andrew (irc:RhodiumToad)


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Re: Request for Patch Feedback: Lag & Lead Window Functions Can Ignore Nulls

[Dean Rasheed]

On 23 May 2016 at 17:01, Jeff Davis  wrote:
> On Fri, May 20, 2016 at 1:41 PM, David G. Johnston
>  wrote:
>> How does the relatively new FILTER clause play into this, if at all?
>
> My interpretation of the standard is that FILTER is not allowable for
> a window function, and IGNORE|RESPECT NULLS is not allowable for an
> ordinary aggregate.
>

That may be so, but we already support FILTER for all windows
functions as well as aggregates:

https://www.postgresql.org/docs/current/static/sql-expressions.html#SYNTAX-AGGREGATES
https://www.postgresql.org/docs/current/static/sql-expressions.html#SYNTAX-WINDOW-FUNCTIONS

so to be clear, what we're talking about here is just supporting SQL
standard syntax for window functions, rather than adding any new
functionality, right?


> So if we support IGNORE|RESPECT NULLS for anything other than a window
> function, we have to come up with our own semantics.
>

Given that we can already do this using FILTER for aggregates, and
that IGNORE|RESPECT NULLS for aggregates is not part of the SQL
standard, I see no reason to support it.

Regards,
Dean


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Re: Request for Patch Feedback: Lag & Lead Window Functions Can Ignore Nulls

[Emre Hasegeli]

> My interpretation of the standard is that FILTER is not allowable for
> a window function, and IGNORE|RESPECT NULLS is not allowable for an
> ordinary aggregate.

Yes, it is clear.

> So if we support IGNORE|RESPECT NULLS for anything other than a window
> function, we have to come up with our own semantics.

I don't think this clause is useful for aggregates especially while we
already have the FILTER clause.  Though, I can see this error message
being useful:

> ERROR:  IGNORE NULLS is only implemented for the lead and lag window functions

Can we still give this message when the syntax is not part of the OVER clause?

Thank you for returning back to this patch.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Re: Request for Patch Feedback: Lag & Lead Window Functions Can Ignore Nulls

[David G. Johnston]

On Mon, May 23, 2016 at 12:01 PM, Jeff Davis  wrote:

> On Fri, May 20, 2016 at 1:41 PM, David G. Johnston
>  wrote:
> > How does the relatively new FILTER clause play into this, if at all?
>
> My interpretation of the standard is that FILTER is not allowable for
> a window function, and IGNORE|RESPECT NULLS is not allowable for an
> ordinary aggregate.
>
> So if we support IGNORE|RESPECT NULLS for anything other than a window
> function, we have to come up with our own semantics.
>
> > We already have "STRICT" for deciding whether a function processes nulls.
> > Wouldn't this need to exist on the "CREATE AGGREGATE"
>
> STRICT defines behavior at DDL time. I was suggesting that we might
> want a DDL-time flag to indicate whether a function can make use of
> the query-time IGNORE|RESPECT NULLS option. In other words, most
> functions wouldn't know what to do with IGNORE|RESPECT NULLS, but
> perhaps some would if we allowed them the option.
>
> Perhaps I didn't understand your point?
>

​The "this" in the quote doesn't refer to STRICT but rather the
non-existence feature that we are talking about.​

​I am trying to make the point that the DDL syntax for "RESPECT|IGNORE
NULLS"​ should be on the "CREATE AGGREGATE" page and not the "CREATE
FUNCTION" page.

As far as a plain function cares its only responsibility with respect to
NULLs is defined by the STRICT property.  As this behavior only manifests
in aggregate situations the corresponding property should be defined there
as well.

David J.

Re: [HACKERS] Re: Request for Patch Feedback: Lag & Lead Window Functions Can Ignore Nulls

[Jeff Davis]

On Fri, May 20, 2016 at 1:41 PM, David G. Johnston
 wrote:
> How does the relatively new FILTER clause play into this, if at all?

My interpretation of the standard is that FILTER is not allowable for
a window function, and IGNORE|RESPECT NULLS is not allowable for an
ordinary aggregate.

So if we support IGNORE|RESPECT NULLS for anything other than a window
function, we have to come up with our own semantics.

> We already have "STRICT" for deciding whether a function processes nulls.
> Wouldn't this need to exist on the "CREATE AGGREGATE"

STRICT defines behavior at DDL time. I was suggesting that we might
want a DDL-time flag to indicate whether a function can make use of
the query-time IGNORE|RESPECT NULLS option. In other words, most
functions wouldn't know what to do with IGNORE|RESPECT NULLS, but
perhaps some would if we allowed them the option.

Perhaps I didn't understand your point?

Regards,
  Jeff Davis


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Re: Request for Patch Feedback: Lag & Lead Window Functions Can Ignore Nulls

[David G. Johnston]

Just doing a drive-by...

On Fri, May 20, 2016 at 3:50 PM, Jeff Davis  wrote:

> Old thread link:
>
> http://www.postgresql.org/message-id/CA+=vxna5_n1q5q5okxc0aqnndbo2ru6gvw+86wk+onsunjd...@mail.gmail.com
>
> On Thu, Apr 14, 2016 at 1:29 PM, Stephen Frost  wrote:
> > Jeff
> >
> > (Reviving an old thread for 2014...)

>
> > Would you have time to work on this for 9.7..?  I came across a
> > real-world use case for exactly this capability and was sorely
> > disappointed to discover we didn't support it even though there had been
> > discussion for years on it, which quite a few interested parties.
> >


> First, I think the syntax is still implemented in a bad way. Right now
> it's part of the OVER clause, and the IGNORE NULLS gets put into the
> frame options. It doesn't match the way the spec defines the grammar,
> and I don't see how it really makes sense that it's a part of the
> frame options or the window object at all.


​How does the relatively new FILTER clause play into this, if at all?

I think we need a need catalog support to say
> whether a function honors IGNORE|RESPECT NULLS or not, which means we
> also need support in CREATE FUNCTION.
>

We already have "STRICT" for deciding whether a function processes nulls.
Wouldn't this need to exist on the "CREATE AGGREGATE"

Rhetorical question: I presume we are going to punt on the issue, since it
is non-standard, but what is supposed to happen with a window invocation
that ignores nulls but has a non-strict function that returns a non-null on
null input?

David J.

[HACKERS] Re: Request for Patch Feedback: Lag & Lead Window Functions Can Ignore Nulls

[Jeff Davis]

Old thread link:
http://www.postgresql.org/message-id/CA+=vxna5_n1q5q5okxc0aqnndbo2ru6gvw+86wk+onsunjd...@mail.gmail.com

On Thu, Apr 14, 2016 at 1:29 PM, Stephen Frost  wrote:
> Jeff
>
> (Reviving an old thread for 2014...)
>
> Would you have time to work on this for 9.7..?  I came across a
> real-world use case for exactly this capability and was sorely
> disappointed to discover we didn't support it even though there had been
> discussion for years on it, which quite a few interested parties.
>
> I'll take a look at reviewing your last version of the patch but wanted
> to get an idea of if you still had interest and might be able to help.
>
> Thanks!
>
> Stephen

There are actually quite a few issues remaining here.

First, I think the syntax is still implemented in a bad way. Right now
it's part of the OVER clause, and the IGNORE NULLS gets put into the
frame options. It doesn't match the way the spec defines the grammar,
and I don't see how it really makes sense that it's a part of the
frame options or the window object at all. I believe that it should be
a part of the FuncCall, and end up in the FuncExpr, so the flag can be
read when the function is called without exposing frame options (which
we don't want to do). I think the reason it was done as a part of the
window object is so that it could save state between calls for the
purpose of optimization, but I think that can be done using fn_extra.
This change should remove a lot of the complexity about trying to
share window definitions, etc.

Second, we need a way to tell which functions support IGNORE NULLS and
which do not. The grammar as implemented in the patch seems to allow
it for any function with an OVER clause (which can include ordinary
aggregates). Then the parse analysis hard-codes that only LEAD and LAG
accept IGNORE NULLS, and throws an error otherwise. Neither of these
things seem right. I think we need a need catalog support to say
whether a function honors IGNORE|RESPECT NULLS or not, which means we
also need support in CREATE FUNCTION.

I think the execution is pretty good, except that (a) we need to keep
the state in fn_extra rather than the winstate; and (b) we should get
rid of the bitmaps and just do a naive scan unless we really think
non-constant offsets will be important. We can always optimize more
later.

Regards,
 Jeff Davis


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] Re: Request for Patch Feedback: Lag & Lead Window Functions Can Ignore Nulls

[Stephen Frost]

Jeff

(Reviving an old thread for 2014...)

* Jeff Davis (pg...@j-davis.com) wrote:
> On Thu, 2014-07-10 at 23:43 -0700, Jeff Davis wrote:
> > On Mon, 2014-07-07 at 01:21 -0700, Jeff Davis wrote:
> > > On Sun, 2014-07-06 at 21:11 -0700, Jeff Davis wrote:
> > > > On Wed, 2014-04-16 at 12:50 +0100, Nicholas White wrote:
> > > > > Thanks for the detailed feedback, I'm sorry it took so long to
> > > > > incorporate it. I've attached the latest version of the patch, fixing
> > > > > in particular:
> > 
> > As innocent as this patch seemed at first, it actually opens up a lot of
> > questions.
> > 
> > Attached is the (incomplete) edit of the patch so far.
> 
> I haven't received much feedback so far on my changes, and I don't think
> I will finish hacking on this in the next few days, so I will mark it
> "returned with feedback".
> 
> I don't think it's too far away, but my changes are substantial enough
> (and incomplete enough) that some feedback is required.

Would you have time to work on this for 9.7..?  I came across a
real-world use case for exactly this capability and was sorely
disappointed to discover we didn't support it even though there had been
discussion for years on it, which quite a few interested parties.

I'll take a look at reviewing your last version of the patch but wanted
to get an idea of if you still had interest and might be able to help.

Thanks!

Stephen


signature.asc
Description: Digital signature

[HACKERS] Re: Request for Patch Feedback: Lag Lead Window Functions Can Ignore Nulls

[Nicholas White]

Hi Abhijit -

 What's the status of this patch?

The latest version of the patch needs a review, and I'd like to get it
committed in this CF if possible. Thanks -

Nick


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] Re: Request for Patch Feedback: Lag Lead Window Functions Can Ignore Nulls

[Abhijit Menon-Sen]

Hi.

What's the status of this patch? Jeff, Álvaro, you're listed as
reviewers. Have you had a chance to look at the updated version
that Nick posted?

-- Abhijit


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Re: Request for Patch Feedback: Lag Lead Window Functions Can Ignore Nulls

[Alvaro Herrera]

We have this block:

+   {
+   /*
+* This is the window we want - but we have to tweak the
+* definition slightly (e.g. to support the IGNORE NULLS frame
+* option) as we're not using the default (i.e. parent) frame
+* options.
+*
+* We'll create a 'child' (using refname to inherit everything
+* from the parent) that just overrides the frame options
+* (assuming it doesn't already exist):
+*/
+   WindowDef  *clone = makeNode(WindowDef);

... then it goes to populate the clone.  When this is done, we use the
clone to walk the list of existing WindowDefs, and if there's a match we
free this one and use that one.  Wouldn't it be better to walk the
existing array first looking for a match, and only create a clone if
none is found?  This would avoid the memory leak problems; I originally
pointed out that you're leaking the bits created by this makeNode() call
above, but now that I look at it again, I think you're also leaking the
bits created by the two copyObject() calls to create the clone.  It
appears to me that it's simpler to not allocate any memory in the first
place, unless necessary.


Also, in parsenodes.h, you had the [MANDATORY] and such tags.  Three
things about that: 1) it looks a lot uglier than the original, so how
about the modified version below?  and 2) what does MANDATORY value of
NULL means?  Maybe you mean MANDATORY value or NULL instead? 3)
Exactly what case does the in this case phrase refer to?  I think the
comment should be more explicit.  Also, I think this should be its own
paragraph instead of being mixed with the For entries in a paragraph.

/*
 * WindowDef - raw representation of WINDOW and OVER clauses
 *
 * For entries in a WINDOW list, name is the window name being defined.
 * For OVER clauses, we use name for the OVER window syntax, or refname
 * for the OVER (window) syntax, which is subtly different --- the latter
 * implies overriding the window frame clause.
 * 
 * In this case, the per-field indicators determine what the semantics
 * are:
 *  [V]irtual
 *  If NULL, then the parent's (refname) value is used.
 *  [M]andatory
 *  Never inherited from the parent, so must be specified; 
may be NULL.
 *  [S]uper
 *  Always inherited from parent, any local version ignored.
 */
typedef struct WindowDef
{
NodeTag type;
char   *name;   /* [M] window's own name */
char   *refname;/* [M] referenced window name, if any */
List   *partitionClause;/* [V] PARTITION BY expression list */
List   *orderClause;/* [M] ORDER BY (list of SortBy) */
int frameOptions;   /* [M] frame_clause options, see below */
Node   *startOffset;/* [M] expression for starting bound, if 
any */
Node   *endOffset;  /* [M] expression for ending bound, if any 
*/
int location;   /* parse location, or -1 if none/unknown */
} WindowDef;

In gram.y there are some spurious whitespaces at end-of-line.  You
should be able to see them with git diff --check.  (I don't think we
support running pgindent on .y files, which would have otherwise cleaned
this up.)

A style issue.  You have this:

+   /*
+* We can process a constant offset much more efficiently; initially
+* we'll scan through the first offset non-null rows, and store that
+* index. On subsequent rows we'll decide whether to push that index
+* forwards to the next non-null value, or just return it again.
+*/
+   leadlag_const_context *context = WinGetPartitionLocalMemory(
+   winobj,
+ sizeof(leadlag_const_context));
+   int count_forward = 0;

I think it'd be better to put the declarations above the comment, and
assignment to context below the comment.  This way, the indentation of
the assignment is not so odd.  So it'd look like

+   leadlag_const_context *context;
+   int count_forward = 0;
+
+   /*
+* We can process a constant offset much more efficiently; initially
+* we'll scan through the first offset non-null rows, and store that
+* index. On subsequent rows we'll decide whether to push that index
+* forwards to the next non-null value, or just return it again.
+*/
+   context = WinGetPartitionLocalMemory(winobj,
+sizeof(leadlag_const_context));


And a final style comment.  You have a block like this:

if (ignore_nulls  !const_offset)
{
long block;
}
else if (ignore_nulls /*  const_offset */)
{
another long block;
}
else
{
more stuff;
}

I think this 

Re: [HACKERS] Re: Request for Patch Feedback: Lag Lead Window Functions Can Ignore Nulls

[Heikki Linnakangas]

On 29.09.2013 23:32, Nicholas White wrote:

bms_add_member() is an accident waiting to happen


I've attached a patch that makes it use repalloc as suggested


You'll have to zero out the extended portion.

I tried to demonstrate that by setting RANDOMIZE_ALLOCATED_MEMORY, but 
surprisingly regression tests still passed. I guess the regression suite 
doesn't use wide enough bitmapsets to exercise that. But this causes an 
assertion failure, with RANDOMIZE_ALLOCATED_MEMORY:


create table t (i int4);
select * from t as t1, t as t2, t as t3, t as t4, t as t5, t as t6, t as 
t7, t as t8, t as t9, t as t10, t as t11, t as t12, t as t13, t as t14, 
t as t15, t as t16, t as t17, t as t18, t as t19, t as t20, t as t21, t 
as t22, t as t23, t as t24, t as t25, t as t26, t as t27, t as t28, t as 
t29, t as t30, t as t31, t as t32, t as t33, t as t34, t as t35, t as 
t36, t as t37, t as t38, t as t39, t as t40;



- is it OK to commit separately? I'll address the lead-lag patch
comments in the next couple of days. Thanks


Yep, thanks. I committed the attached.

After thinking about this some more, I realized that bms_add_member() is 
still sensitive to CurrentMemoryContext, if the 'a' argument is NULL. 
That's probably OK for the laglead patch - I didn't check - but if 
we're going to start relying on the fact that bms_add_member() no longer 
allocates a new bms in CurrentMemoryContext, that needs to be 
documented. bitmapset.c currently doesn't say a word about memory contexts.


So what needs to be done next is to document how the functions in 
bitmapset.c work wrt. memory contexts. Then double-check that the 
behavior of all the other recycling bms functions is consistent. (At 
least bms_add_members() needs a similar change).


- Heikki
From ee01d848f39400c8524c66944ada6fde47894978 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas heikki.linnakan...@iki.fi
Date: Mon, 30 Sep 2013 16:37:00 +0300
Subject: [PATCH 1/1] In bms_add_member(), use repalloc() if the bms needs to
 be enlarged.

Previously bms_add_member() would palloc a whole-new copy of the existing
set, copy the words, and pfree the old one. repalloc() is potentially much
faster, and more importantly, this is less surprising if CurrentMemoryContext
is not the same as the context the old set is in. bms_add_member() still
allocates a new bitmapset in CurrentMemoryContext if NULL is passed as
argument, but that is a lot less likely to induce bugs.

Nicholas White.
---
 src/backend/nodes/bitmapset.c | 19 +--
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/src/backend/nodes/bitmapset.c b/src/backend/nodes/bitmapset.c
index b18b7a5..540db16 100644
--- a/src/backend/nodes/bitmapset.c
+++ b/src/backend/nodes/bitmapset.c
@@ -632,21 +632,20 @@ bms_add_member(Bitmapset *a, int x)
 		return bms_make_singleton(x);
 	wordnum = WORDNUM(x);
 	bitnum = BITNUM(x);
+
+	/* enlarge the set if necessary */
 	if (wordnum = a-nwords)
 	{
-		/* Slow path: make a larger set and union the input set into it */
-		Bitmapset  *result;
-		int			nwords;
+		int			oldnwords = a-nwords;
 		int			i;
 
-		result = bms_make_singleton(x);
-		nwords = a-nwords;
-		for (i = 0; i  nwords; i++)
-			result-words[i] |= a-words[i];
-		pfree(a);
-		return result;
+		a = (Bitmapset *) repalloc(a, BITMAPSET_SIZE(wordnum + 1));
+		a-nwords = wordnum + 1;
+		/* zero out the enlarged portion */
+		for (i = oldnwords; i  a-nwords; i++)
+			a-words[i] = 0;
 	}
-	/* Fast path: x fits in existing set */
+
 	a-words[wordnum] |= ((bitmapword) 1  bitnum);
 	return a;
 }
-- 
1.8.4.rc3


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Re: Request for Patch Feedback: Lag Lead Window Functions Can Ignore Nulls

[Alvaro Herrera]

Nicholas White escribió:

  But even if we did decide to switch memory contexts on every call, it would 
  still be much cleaner than this.
 
 I've removed all the bms_initalize code from the patch and am using
 this solution. As the partition memory is zero-initialised I just
 store a Bitmapset pointer in the WinGetPartitionLocalMemory. The
 bms_add_member and bms_is_member functions behave sensibly for
 null-pointer inputs (they return a bms_make_singleton in the current
 memory context and false respectively). I've surrounded the calls to
 bms_make_singleton with a memory context switch (to the partition's
 context) so the Bitmapset stays in the partition's context.

Now that I look again, would GetMemoryChunkContext() be useful here?

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training  Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Re: Request for Patch Feedback: Lag Lead Window Functions Can Ignore Nulls

[Nicholas White]

 bms_add_member() is an accident waiting to happen

I've attached a patch that makes it use repalloc as suggested - is it
OK to commit separately? I'll address the lead-lag patch comments in
the next couple of days. Thanks -


repalloc.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Re: Request for Patch Feedback: Lag Lead Window Functions Can Ignore Nulls

[Robert Haas]

On Thu, Sep 26, 2013 at 4:20 PM, Alvaro Herrera
alvhe...@2ndquadrant.com wrote:
 But how do we ensure that the BMS is allocated in a context?  You'd have
 to switch contexts each time you call bms_add_member.  I don't have a
 good answer to this.

The coding of bms_add_member is pretty funky.  Why doesn't it just
repalloc() the input argument if it's not big enough?  If it did that,
the new allocation would be in the same memory context as the original
one, and we'd not need to care about the memory context when adding an
element to an already non-empty set.

But even if we did decide to switch memory contexts on every call, it
would still be much cleaner than this.  It's only three lines of code
(and about as many instructions) to save and restore
CurrentMemoryContext.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Re: Request for Patch Feedback: Lag Lead Window Functions Can Ignore Nulls

[Heikki Linnakangas]

On 27.09.2013 14:12, Robert Haas wrote:

On Thu, Sep 26, 2013 at 4:20 PM, Alvaro Herrera
alvhe...@2ndquadrant.com  wrote:

But how do we ensure that the BMS is allocated in a context?  You'd have
to switch contexts each time you call bms_add_member.  I don't have a
good answer to this.


The coding of bms_add_member is pretty funky.  Why doesn't it just
repalloc() the input argument if it's not big enough?  If it did that,
the new allocation would be in the same memory context as the original
one, and we'd not need to care about the memory context when adding an
element to an already non-empty set.

But even if we did decide to switch memory contexts on every call, it
would still be much cleaner than this.  It's only three lines of code
(and about as many instructions) to save and restore
CurrentMemoryContext.


[looks] Huh, yeah, as it stands, bms_add_member() is an accident waiting 
to happen. It's totally non-obvious that it might cause the BMS to jump 
from another memory context to CurrentMemoryContext. Same with 
bms_add_members(). And it would be good to Assert in bms_join that both 
arguments are in the same from MemoryContext.


Let's fix that...

- Heikki


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Re: Request for Patch Feedback: Lag Lead Window Functions Can Ignore Nulls

[Alvaro Herrera]

I gave this a quick look.  It took me a while to figure out how to apply
it -- turns out you used the ignore whitespace option to diff, so the
only way to apply it was with patch -p1 --ignore-whitespace.  Please
don't do that.

I ran pgindent over the patched code and there were a number of changes.
I suggest you run that over your local copy before your next submission,
to avoid the next official run to mangle your stuff in unforeseen ways.
For instance, the comment starting with A slight edge case would be
mangled; I suggest enclosing that in /* to avoid the problem.
(TBH that's the only interesting thing, but avoiding that kind of
breakage is worth it IMHO.)

First thing I noticed was the funky bms_initialize thingy.  There was
some controversy upthread about the use of bitmapsets, and it seems you
opted for not using them for the constant case as suggested by Jeff; but
apparently the other comment by Robert about the custom bms initializer
went largely ignored.  I agree with him that this is grotty.  However,
the current API to get partition-local memory is too limited as there's
no way to change to the partition's context; instead you only get the
option to allocate a certain amount of memory and return that.  I think
the easiest way to get around this problem is to create a new
windowapi.h function which returns the MemoryContext for the partition.
Then you can just allocate the BMS in that context.

But how do we ensure that the BMS is allocated in a context?  You'd have
to switch contexts each time you call bms_add_member.  I don't have a
good answer to this.  I used this code in another project:

/*
 * grow the visited bitmapset to the index' current size, to avoid
 * repeated repalloc's
 */
{
BlockNumber lastblock;

lastblock = RelationGetNumberOfBlocks(rel);
visited = bms_add_member(visited, lastblock);
visited = bms_del_member(visited, lastblock);
}

This way, I know the bitmapset already has enough space for all the bits
I need and there will be no further allocation.  But this is also
grotty.  Maybe we should have a new entry point in bitmapset.h, say
bms_grow that ensures you have enough space for that many bits.  Or
perhaps add a MemoryContext member to struct Bitmapset, so that all
allocations occur therein.

I'm not too sure I follow the parse_agg.c changes, but don't you need to
free the clone in the branch that finds a duplicate window spec?  Is
this parent/child relationship thingy a preexisting concept, or are you
just coming up with it?  It seems a bit unfamiliar to me.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training  Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Re: Request for Patch Feedback: Lag Lead Window Functions Can Ignore Nulls

[Peter Eisentraut]

On Wed, 2013-08-21 at 22:34 -0400, Nicholas White wrote:
  but needs a rebase.
 
 See attached - thanks!

Please fix these compiler warnings:

windowfuncs.c: In function ‘leadlag_common’:
windowfuncs.c:366:3: warning: passing argument 1 of ‘bms_initialize’ from 
incompatible pointer type [enabled by default]
In file included from windowfuncs.c:16:0:
../../../../src/include/nodes/bitmapset.h:97:19: note: expected ‘void * 
(*)(void *, Size)’ but argument is of type ‘void * (*)(struct WindowObjectData 
*, Size)’
windowfuncs.c:306:8: warning: ‘result’ may be used uninitialized in this 
function [-Wmaybe-uninitialized]




-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Re: Request for Patch Feedback: Lag Lead Window Functions Can Ignore Nulls

[Nicholas White]

 Please fix these compiler warnings

Fixed - see attached. Thanks -


lead-lag-ignore-nulls.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Re: Request for Patch Feedback: Lag Lead Window Functions Can Ignore Nulls

[Peter Eisentraut]

On Thu, 2013-07-11 at 10:51 -0400, Nicholas White wrote:
 I've attached a revised version that fixes the issues above:

This patch is in the 2013-09 commitfest but needs a rebase.



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Re: Request for Patch Feedback: Lag Lead Window Functions Can Ignore Nulls

[Nicholas White]

 but needs a rebase.

See attached - thanks!


lead-lag-ignore-nulls.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Re: Request for Patch Feedback: Lag Lead Window Functions Can Ignore Nulls

[Heikki Linnakangas]

On 21.07.2013 08:41, Jeff Davis wrote:

(For that matter, am I not supposed to commit between 'fests? Or is it
still an option for me to finish up with this after I get back even if
we close the CF?)


It's totally OK to commit stuff between 'fests.

- Heikki


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Re: Request for Patch Feedback: Lag Lead Window Functions Can Ignore Nulls

[Josh Berkus]

 (For that matter, am I not supposed to commit between 'fests? Or is it
 still an option for me to finish up with this after I get back even if
 we close the CF?)

The idea of the CommitFests is to give committers some *time off*
between them.  If a committer wants to commit stuff when it's not a CF,
that's totally up to them.

-- 
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Re: Request for Patch Feedback: Lag Lead Window Functions Can Ignore Nulls

[Jeff Davis]

On Fri, 2013-07-19 at 09:39 -0700, Josh Berkus wrote:
 So ... are you doing a final review of this for the CF, Jeff?  We need
 to either commit it or bounce it to the next CF.

I am going on vacation tomorrow, and I just didn't quite find time to
take this to commit. Sorry about that, Nicholas. The patch improved a
lot this CF though, so we'll get it in quickly and I don't foresee any
problem with it making it in 9.4.

(For that matter, am I not supposed to commit between 'fests? Or is it
still an option for me to finish up with this after I get back even if
we close the CF?)

Regards,
Jeff Davis





-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Re: Request for Patch Feedback: Lag Lead Window Functions Can Ignore Nulls

[Josh Berkus]

On 07/15/2013 10:19 AM, Jeff Davis wrote:
 On Thu, 2013-07-11 at 10:51 -0400, Nicholas White wrote:
 I've attached a revised version that fixes the issues above:
 
 I'll get to this soon, sorry for the delay.
 
 Regards,
   Jeff Davis

So ... are you doing a final review of this for the CF, Jeff?  We need
to either commit it or bounce it to the next CF.

-- 
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] Re: Request for Patch Feedback: Lag Lead Window Functions Can Ignore Nulls

[Jeff Davis]

On Thu, 2013-07-11 at 10:51 -0400, Nicholas White wrote:
 I've attached a revised version that fixes the issues above:

I'll get to this soon, sorry for the delay.

Regards,
Jeff Davis




-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] Re: Request for Patch Feedback: Lag Lead Window Functions Can Ignore Nulls

[Nicholas White]

np, optimising for quality not speed :)


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] Re: Request for Patch Feedback: Lag Lead Window Functions Can Ignore Nulls

[Nicholas White]

I've attached a revised version that fixes the issues above:

 changing a reference of the form:
   OVER w
 into:
   OVER (w)

Fixed (and I've updated the tests).

 It's bad form to modify a list while iterating through it.

Fixed

 We shouldn't create an arbitrary number of duplicate windows

Fixed

 Is there a problem with having two windowdefs in
 the p_windowdefs list with the same name
 ...
 You'll have to be a little careful that any other code knows that names
 can be duplicated in the list though.

I'm not sure I really can verify this - as I'm not sure how much
contrib / other third-party code has access to this data structure.
I'd prefer to be cautious and just create a child window if needed.

 I think we should get rid of the bitmapset entirely
 ...
 Instead of the bitmapset, we can keep track of two offsets

I've modified leadlag_common so it uses your suggested algorithm for
constant offsets (although it turns out you only need to keep a single
int64 index in the context). This algorithm calls
WinGetFuncArgInPartition at least twice per row, once to check whether
the current row is null (and so check if we have to move the leading /
lagged index forward) and either once to get leading / lagging value
or more than once to push the leading / lagged value forwards to the
next non-null value.
I've kept the bitmap solution for the non-constant offset case (i.e.
the random partition access case) as I believe it changes the cost of
calculating the lead / lagged values for every row in the partition to
O(partition size) - whereas a non-caching scan-the-partition solution
would be O(partition size * partition size). Is that OK?

Thanks -

Nick


lead-lag-ignore-nulls.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] Re: Request for Patch Feedback: Lag Lead Window Functions Can Ignore Nulls

[Jeff Davis]

On Mon, 2013-07-01 at 18:20 -0400, Nicholas White wrote:
  pg_get_viewdef() needs to be updated
 
 Ah, good catch - I've fixed this in the attached. I also discovered
 that there's a parent-child hierarchy of WindowDefs (using
 relname-name), so instead of cloning the WindowDef (in parse_agg.c)
 if the frameOptions are different (e.g. by adding the ignore-nulls
 flag) I create a child of the WindowDef and override the frameOptions.
 This has the useful side-effect of making pg_get_viewdef work as
 expected (the previous iteration of the patch produced a copy of the
 window definintion, not the window name, as it was using a nameless
 clone), although the output has parentheses around the view name:
 
A couple comments:
 * We shouldn't create an arbitrary number of duplicate windows when
many aggregates are specified with IGNORE NULLS.
 * It's bad form to modify a list while iterating through it. This is
just a style issue because there's a break afterward, anyway.

Also, I'm concerned that we're changing a reference of the form:
  OVER w
into:
  OVER (w)
in a user-visible way. Is there a problem with having two windowdefs in
the p_windowdefs list with the same name and different frameOptions?

I think you could just change the matching criteria to be a matching
name and matching frameOptions. In the loop, if you find a matching name
but frameOptions doesn't match, keep a pointer to the windowdef and
create a new one at the end of the loop with the same name.

You'll have to be a little careful that any other code knows that names
can be duplicated in the list though.

Regards,
Jeff Davis




-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers