Re: Optimize WindowAgg's use of tuplestores

Hi David,

Thank you for the patch.

> I think this might just be noise as a result of rearranging code. In
> terms of C code, I don't see any reason for it to be slower. If you
> look at GenerationDelete() (as what is getting called from
> MemoryContextDelete()), it just calls GenerationReset(). So resetting
> is going to always be less work than deleting the context, especially
> given we don't need to create the context again when we reset it.
>
> I wrote the attached script to see if I can also see the slowdown and
> I do see the patched code come out slightly slower (within noise
> levels) in lower partition counts.
>
> To get my compiler to produce code in a more optimal order for the
> common case, I added unlikely() to the "if (winstate->all_first)"
> condition. This is only evaluated on the first time after a rescan,
> so putting that code at the end of the function makes more sense. The
> attached v2 patch has it this way. You can see the numbers look
> slightly better in the attached graph.
>
> Thanks for having a look at this.
>
> David

@@ -2684,6 +2726,14 @@ ExecEndWindowAgg(WindowAggState *node)
PlanState *outerPlan;
int i;

+ if (node->buffer != NULL)
+ {
+ tuplestore_end(node->buffer);
+
+ /* nullify so that release_partition skips the tuplestore_clear() */
+ node->buffer = NULL;
+ }
+

Is it possible that node->buffer == NULL in ExecEndWindowAgg()? If
not, probably making it an Assert() or just removing the if() should
be fine.

Best reagards,
--
Tatsuo Ishii
SRA OSS LLC
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp