Re: Rethinking representation of partial-aggregate steps

On 24 June 2016 at 05:12, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> I do agree, however, that the three Boolean flags don't make the code
> entirely easy to read. What I might suggest is that we replace the
> three Boolean flags with integer flags, something like this:
>
> #define AGGOP_COMBINESTATES 0x1
> #define AGGOP_SERIALIZESTATES 0x2
> #define AGGOP_FINALIZEAGGS 0x4
>
> #define AGGTYPE_SIMPLE (AGGOP_FINALIZEAGGS)
> #define AGGTYPE_PARTIAL_SERIAL (AGGOP_SERIALIZESTATES)
> #define AGGTYPE_FINALIZE_SERIAL
> (AGGOP_COMBINESTATES|AGGOP_FINALIZEAGGS|AGG_SERIALIZESTATES)
>
> Code that requests behavior can do so with the AGGTYPE_* constants,
> but code that implements behavior can test AGGOP_* constants. Then if
> we decide we need new combinations in the future, we can just add
> those --- e.g. AGGTYPE_PARTIAL = 0, AGGTYPE_FINALIZE =
> AGGOP_COMBINESTATES|AGGOP_FINALIZEAGGS, AGGTYPE_INTERMEDIATE =
> AGGOP_COMBINESTATES --- and have some hope that it will mostly work.
>
> I actually think nearly all of the combinations are sensible, here,
> although I think it may have been a mistake to overload the "serialize
> states" flag to mean both "does this combining aggregate expect to
> receive the serial type rather than the transition type?" and also
> "does this partial aggregate output the serial type rather than the
> transition type?". In the example above, you'd want the
> IntermediateAggregate to expect the transition type but output the
> serial type. Oops.

I did consider using bit flags for this before, and it's a little bit
of an accident that it didn't end up that way. Originally there were
just two bool flags to control finalize and combine. A later patch
added the serialisation stuff which required the 3rd flag. It then
became a little untidy, but I hesitated to change it as I really just
wanted to keep the patch as easy to review as possible. In hindsight,
if I should've probably used bit flags from day one.

As for the above proposal, I do agree that it'll be cleaner with bit
flags, I just really don't see the need for the AGGTYPE_* alias
macros. For me it's easier to read if each option is explicitly listed
similar to how pull_var_clause() is done, e.g:

List *vars = pull_var_clause((Node *) cur_em->em_expr,
PVC_RECURSE_AGGREGATES |
PVC_RECURSE_WINDOWFUNCS |
PVC_INCLUDE_PLACEHOLDERS);

I'll start by writing the code to do that much, and if the consensus
is to add the alias macros too, then it's a small addition.

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services