Thread: Remove a unused argument from qual_is_pushdown_safe

Hello,

I found that qual_is_pushdown_safe() has an argument "subquery"
that is not used in the function.  This argument has not been
referred to since the commit 964c0d0f80e485dd3a4073e073ddfd9bfdda90b2.

I think we can remove this if there is no special reason. 

Regards,
Yugo Nagata 

-- 
Yugo NAGATA <nagata@sraoss.co.jp>

On Fri, Nov 25, 2022 at 04:05:13PM +0800, Richard Guo wrote:
> +1.  In 964c0d0f the checks in qual_is_pushdown_safe() that need to
> reference 'subquery' were moved to subquery_is_pushdown_safe(), so param
> 'subquery' is not used any more.  I think we can just remove it.
>
> I wonder if we need to revise the comment atop qual_is_pushdown_safe()
> too which says
>
>  * rinfo is a restriction clause applying to the given subquery (whose RTE
>  * has index rti in the parent query).
>
> since there is no 'given subquery' after we remove it from the params.

When it comes to specific subpaths of the tree, it is sometimes good
to keep some symmetry in the arguments of the sub-routines used, but
that does not seem to apply much to allpaths.c.  Removing that is fine
by me, so let's do this.
--
Michael

On Mon, Nov 28, 2022 at 11:54:45AM +0900, Michael Paquier wrote:
> On Fri, Nov 25, 2022 at 04:05:13PM +0800, Richard Guo wrote:
>> I wonder if we need to revise the comment atop qual_is_pushdown_safe()
>> too which says
>>
>>  * rinfo is a restriction clause applying to the given subquery (whose RTE
>>  * has index rti in the parent query).
>>
>> since there is no 'given subquery' after we remove it from the params.

I was thinking about this point, and it seems to me that we could just
do s/the given subquery/a subquery/.  But perhaps you have a different
view on the matter?
--
Michael

On Mon, 28 Nov 2022 16:40:52 +0900
Michael Paquier <michael@paquier.xyz> wrote:

> On Mon, Nov 28, 2022 at 11:54:45AM +0900, Michael Paquier wrote:
> > On Fri, Nov 25, 2022 at 04:05:13PM +0800, Richard Guo wrote:
> >> I wonder if we need to revise the comment atop qual_is_pushdown_safe()
> >> too which says
> >> 
> >>  * rinfo is a restriction clause applying to the given subquery (whose RTE
> >>  * has index rti in the parent query).
> >> 
> >> since there is no 'given subquery' after we remove it from the params.
> 
> I was thinking about this point, and it seems to me that we could just
> do s/the given subquery/a subquery/.  But perhaps you have a different
> view on the matter?

+1
I also was just about to send a patch updated as so, and this is attached.

Regards,
Yugo Nagata

> --
> Michael


-- 
Yugo NAGATA <nagata@sraoss.co.jp>

Michael Paquier <michael@paquier.xyz> writes:
> On Mon, Nov 28, 2022 at 11:54:45AM +0900, Michael Paquier wrote:
>> On Fri, Nov 25, 2022 at 04:05:13PM +0800, Richard Guo wrote:
> I wonder if we need to revise the comment atop qual_is_pushdown_safe()
> too which says
> 
> * rinfo is a restriction clause applying to the given subquery (whose RTE
> * has index rti in the parent query).
> 
> since there is no 'given subquery' after we remove it from the params.

> I was thinking about this point, and it seems to me that we could just
> do s/the given subquery/a subquery/.  But perhaps you have a different
> view on the matter?

My viewpoint is that this change is misguided.  Even if the current
coding of qual_is_pushdown_safe doesn't happen to reference the
subquery, it might need to in future.

            regards, tom lane

> On 28 Nov 2022, at 15:15, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> My viewpoint is that this change is misguided.  Even if the current
> coding of qual_is_pushdown_safe doesn't happen to reference the
> subquery, it might need to in future.

If I understand the code correctly the variable has some value in terms of
"documenting the code" (for lack of better terminology), and I would assume
virtually every modern compiler to figure out it's not needed.

--
Daniel Gustafsson        https://vmware.com/