On 24 June 2016 at 05:12, Robert Haas <robertmh...@gmail.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers