-----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.rouh...@dalibo.com <mailto:julien.rouh...@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----- -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
