Re: 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 <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 looks better like this, even if it causes an extra level of
indentation:

if (ignore_nulls)
{
if (const_offset)
{
some stuff;
}
else
{
more;
}
}
else
{
the third block;
}

Finally, I'm not really sure about the column you added to the
regression tests table. It looks way too artificial; I mean the column
name even states what test is going to use that data (respect=gorilla?
uhm). I'm not sure what's a better option; maybe if you just named the
column "favorite_pet" or something like that, it would appear less
random. Maybe it'd be better to just create your own table for this.

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