Re: FIX : teach expression walker about RestrictInfo

On 03/18/2016 08:53 PM, Robert Haas wrote:
> On Fri, Mar 18, 2016 at 3:29 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Robert Haas <robertmhaas@gmail.com> writes:
>>> On Wed, Apr 29, 2015 at 12:33 PM, Tomas Vondra
>>> <tomas.vondra@2ndquadrant.com> wrote:
>>>> On 04/29/15 18:26, Tom Lane wrote:
>>>>> But there are basic reasons why expression_tree_walker should not try
>>>>> to deal with RestrictInfos; the most obvious one being that it's not
>>>>> clear whether it should descend into both the basic and OR-clause
>>>>> subtrees. Also, if a node has expression_tree_walker support then it
>>>>> should logically have expression_tree_mutator support as well, but
>>>>> that's got multiple issues. RestrictInfos are not supposed to be
>>>>> copied once created; and the mutator couldn't detect whether their
>>>>> derived fields are still valid.
>>
>>>> OK, I do understand that. So what about pull_varnos_walker and
>>>> pull_varattnos_walker - what about teaching them about RestrictInfos?
>>
>>> This patch has become part 1 of many under the "multivariate
>>> statistics vNNN" family of threads, but I guess it would be helpful if
>>> you could opine on the reasonable-ness of this approach.  I don't want
>>> to commit anything over your objections, but this kind of tailed off
>>> without a conclusion.
>>
>> I'm up to my ears in psql at the moment, but hope to get to the
>> multivariate stats patch later, maybe next week.
>
> Oh, cool.
>
>> In the meantime,
>> I remain of the opinion that RestrictInfo is not an expression node
>> and wanting expression_tree_walker to handle it is wrong.  I'm
>> suspicious about pull_varnos; it might be all right given that we
>> can assume the same Vars are in both subtrees, but I still don't
>> really see why that one function has suddenly grown this need when
>> others have not.
>
> I haven't studied the patch series in enough detail to have an
> educated opinion on that.

FWIW I'm not convinced this part of the patch (matching the clauses to 
the available stats, including the pull_varnos changes) is perfectly 
correct either, and it's quite possible it will need reworking. But it 
needs a pair of fresh eyes, I guess.

regards

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