Subject: Re: PoC/WIP: Extended statistics on expressions
To: Dean Rasheed <dean.a.rasheed@gmail.com>
Cc: Justin Pryzby <pryzby@telsasoft.com>,
 PostgreSQL Hackers <pgsql-hackers@postgresql.org>
References: <f0f7e00c-7ccb-9412-0147-a06a60777f06@enterprisedb.com>
 <CAEZATCWF66BWJq-OwLuP5LGK6W9LDYxCQwLxqB36qmq3b1Ch8Q@mail.gmail.com>
 <f4dac079-6bc4-ccc0-0fd8-3e2e2da28d92@enterprisedb.com>
 <CAEZATCXz2WO9ZctRL68_HrsPbpre0GRmip83TgznoeLoWm4=Dg@mail.gmail.com>
 <CAEZATCXcX0gm2LMb+xObroS-C+a5FXEqgR5RgQ9NWKjPC=9sNA@mail.gmail.com>
 <ce50ee26-7135-c435-cae8-073ad59eba60@enterprisedb.com>
 <CAEZATCWgZPJGbj2ndXsoD6_MuJ=H1Y-gdTMdkE=n1VsA6WX+RA@mail.gmail.com>
 <9f6654b1-ca31-f979-0fe8-137683818273@enterprisedb.com>
 <20210324081402.GB28335@telsasoft.com>
 <b8359875-b91f-ff48-4c6c-9b3c1c35d880@enterprisedb.com>
 <20210324091708.GC28335@telsasoft.com>
 <1ff3fc47-8f6e-7839-c3ff-6b694956f92e@enterprisedb.com>
 <CAEZATCWYCY=2yEyxa6BwimCPQ_RZHy5GOb6U6i6dGHGQdjQESQ@mail.gmail.com>
From: Tomas Vondra <tomas.vondra@enterprisedb.com>
Message-ID: <e4556e47-940d-58a6-73fd-16c5815dd2cd@enterprisedb.com>
Date: Wed, 24 Mar 2021 15:47:54 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.8.1
MIME-Version: 1.0
In-Reply-To: 
 <CAEZATCWYCY=2yEyxa6BwimCPQ_RZHy5GOb6U6i6dGHGQdjQESQ@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/e4556e47-940d-58a6-73fd-16c5815dd2cd%40enterprisedb.com>
Precedence: bulk

On 3/24/21 2:36 PM, Dean Rasheed wrote:
> On Wed, 24 Mar 2021 at 10:22, Tomas Vondra
> <tomas.vondra@enterprisedb.com> wrote:
>>
>> Thanks, it seems to be some thinko in handling in PlaceHolderVars, which
>> seem to break the code's assumptions about varnos. This fixes it for me,
>> but I need to look at it more closely.
>>
> 
> I think that makes sense.
> 

AFAIK the primary issue here is that the two places disagree. While
estimate_num_groups does this

    varnos = pull_varnos(root, (Node *) varshere);
    if (bms_membership(varnos) == BMS_SINGLETON)
    { ... }

the add_unique_group_expr does this

    varnos = pull_varnos(root, (Node *) groupexpr);

That is, one looks at the group expression, while the other look at vars
extracted from it by pull_var_clause(). Apparently for PlaceHolderVar
this can differ, causing the crash.

So we need to change one of those places - my fix tweaked the second
place to also look at the vars, but maybe we should change the other
place? Or maybe it's not the right fix for PlaceHolderVars ...

> Reviewing the docs, I noticed a couple of omissions, and had a few
> other suggestions (attached).
> 

Thanks! I'll include that in the next version of the patch.


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company