On 03/13/2016 10:44 PM, David Rowley wrote:
On 12 March 2016 at 16:31, David Rowley <david.row...@2ndquadrant.com> wrote:
I've attached an updated patch which is based on commit 7087166,
things are really changing fast in the grouping path area at the
moment, but hopefully the dust is starting to settle now.

The attached patch fixes a harmless compiler warning about a
possible uninitialised variable.

I've looked at this patch today. The patch seems quite solid, but I do have a few minor comments (or perhaps questions, given that this is the first time I looked at the patch).

1) exprCollation contains this bit:

    case T_PartialAggref:
        coll = InvalidOid; /* XXX is this correct? */

I doubt this is the right thing to do. Can we actually get to this piece of code? I haven't tried too hard, but regression tests don't seem to trigger this piece of code.

Moreover, if we're handling PartialAggref in exprCollation(), maybe we should handle it also in exprInputCollation and exprSetCollation?

And if we really need the collation there, why not to fetch the actual collation from the nested Aggref? Why should it be InvalidOid?

2) partial_aggregate_walker

I think this should follow the naming convention that clearly identifies the purpose of the walker, not what kind of nodes it is supposed to walk. So it should be:


3) create_parallelagg_path

I do agree with the logic that under-estimates are more dangerous than over-estimates, so the current estimate is safer. But I think this would be a good place to apply the formula I proposed a few days ago (or rather the one Dean Rasheed proposed in response).

That is, we do know that there are numGroups in total, and each parallel worker sees subpath->rows then it's expected to see

    sel = (subpath->rows / rel->tuples);
    perGroup = (rel->tuples / numGroups);
    workerGroups = numGroups * (1 - powl(1 - s, perGroup));
    numPartialGroups = numWorkers * workerGroups

It's probably better to see Dean's message from 13/3.

4) Is clauses.h the right place for PartialAggType?


Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to