Re: Removing Functionally Dependent GROUP BY Columns - Mailing list pgsql-hackers

On 14/01/2016 01:30, David Rowley wrote:
> Many thanks for the thorough review!
> 

And thanks for the patch and fast answer!

> On 12 January 2016 at 23:37, Julien Rouhaud <julien.rouhaud@dalibo.com
> <mailto:julien.rouhaud@dalibo.com>> wrote:
> 
> 
>     In identify_key_vars()
> 
>     +       /* Only PK constraints are of interest for now, see comment
>     above */
>     +       if (con->contype != CONSTRAINT_PRIMARY)
>     +           continue;
> 
>     I think the comment should better refer to
>     remove_useless_groupby_columns() (don't optimize further than what
>     check_functional_grouping() does).
> 
> 
> I've improved this by explaining it more clearly.
>  

Very nice.

Small typo though:


+         * Technically we could look at UNIQUE indexes too, however we'd also
+         * have to ensure that each column of the unique index had a NOT NULL


s/had/has/


+         * constraint, however since NOT NULL constraints currently are don't


s/are //

> 
> [...]
> 
> 
>     +               {
>     +                   varlistmatches = bms_add_member(varlistmatches,
>     varlistidx);
>     +                   found_col = true;
>     +                   /* don't break, there might be dupicates */
>     +               }
> 
>     small typo on "dupicates". Also, I thought transformGroupClauseExpr()
>     called in the parse analysis make sure that there isn't any duplicate in
>     the GROUP BY clause. Do I miss something?
> 
> 
> No I missed something :) You are right. I've put a break; here and a
> comment to explain why.
> 

ok :)


> [...]
> 
> 
>     +   /*
>     +    * If we found any surplus Vars in the GROUP BY clause, then
>     we'll build
>     +    * a new GROUP BY clause without these surplus Vars.
>     +    */
>     +   if (anysurplus)
>     +   {
>     +       List *new_groupby = NIL;
>     +
>     +       foreach (lc, root->parse->groupClause)
>     +       {
>     +           SortGroupClause *sgc = (SortGroupClause *) lfirst(lc);
>     +           TargetEntry *tle;
>     +           Var *var;
>     +
>     +           tle = get_sortgroupclause_tle(sgc, root->parse->targetList);
>     +           var = (Var *) tle->expr;
>     +
>     +           if (!IsA(var, Var))
>     +               continue;
>     + [...]
> 
>     if var isn't a Var, it needs to be added in new_groupby.
> 
> 
> Yes you're right, well, at least I've written enough code to ensure that
> it's not needed.

Oh yes, I realize that now.

> Technically we could look inside non-Vars and check if the Expr is just
> made up of Vars from a single relation, and then we could also make that
> surplus if we find other Vars which make up the table's primary key.  I
> didn't make these changes as I thought it was a less likely scenario. It
> wouldn't be too much extra code to add however. I've went and added an
> XXX comment to explain that there might be future optimisation
> opportunities in the future around this. 
>  

Agreed.

> I've attached an updated patch.
> 

+        /* don't try anything unless there's two Vars */
+        if (varlist == NULL || list_length(varlist) < 2)
+            continue;

To be perfectly correct, the comment should say "at least two Vars".

Except the small remaining typos, this patch looks very fine to me. I
flag it as ready for committer.

-- 
Julien Rouhaud
http://dalibo.com - http://dalibo.org