Re: [HACKERS] Re: Request for Patch Feedback: Lag & Lead Window Functions Can Ignore Nulls
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
> "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
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
> 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
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
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
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
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
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
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
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
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 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 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 t
Re: [HACKERS] Re: Request for Patch Feedback: Lag & Lead Window Functions Can Ignore Nulls
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
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 lag&lead 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 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
> 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
On 27.09.2013 14:12, Robert Haas wrote: On Thu, Sep 26, 2013 at 4:20 PM, Alvaro Herrera 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
On Thu, Sep 26, 2013 at 4:20 PM, Alvaro Herrera 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
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
> 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
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
> 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
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
> (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
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
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
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
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
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
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
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