Re: Removing Functionally Dependent GROUP BY Columns

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 14/01/2016 23:05, David Rowley wrote:
> On 15 January 2016 at 00:19, Julien Rouhaud
> <julien.rouhaud@dalibo.com <mailto:julien.rouhaud@dalibo.com>>
> wrote:
> 
> 
> +                * 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 //
> 
> 
> Both fixed. Thanks.
> 
> 
>> +   /* +    * 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.
> 
> 
> I meant to say "I've not written enough code" ...
> 

Yes, that makes sense with the explanation you wrote just after :)

> 
>> 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".
> 
> 
> Changed per discussion from you and Geoff
> 
> 
> Except the small remaining typos, this patch looks very fine to me.
> I flag it as ready for committer.
> 
> 
> Many thanks for your followup review.
> 
> I've attached an updated patch to address what you highlighted
> above.
> 

Thanks!

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


- -- 
Julien Rouhaud
http://dalibo.com - http://dalibo.org
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.17 (GNU/Linux)

iQEcBAEBAgAGBQJWmCoYAAoJELGaJ8vfEpOqJzIIALajMHHd8aCDnAuH9uNUyevU
EuKyHTRDJkc8KUNbtDeSpVf9UGT3XUaZx4k/o+aKDdRB/RfYK0GKyDv2Owr4Wx3F
5BY9GuEO3vjqaFuDBH5u9EjySal3jQYC57nB3I0hRvpVRQBi0nFyQre/SXplCB6q
X1NqBfICyu6lwwocAMCeW9qN4ZQclLjxoScJpA4ml9Kj6CQvK2dDSyS00gstLFPH
Bj+n20wEcC7ZyxCpxfHmoZW1sjAvZK5mwrEdFG+lvCO8OwN/73YvDFzQHrpvVXWE
ZVoz069kfekZtXQ1OQ5CcvOAJLD9ewbPq+rpKh9YQorZB1R9QEj0qaxqo7LtjhA=
=G/uH
-----END PGP SIGNATURE-----