From: Tom Lane <tgl@sss.pgh.pa.us>
To: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
cc: David Rowley <david.rowley@2ndquadrant.com>,
        "Imai,
    Yoshikazu" <imai.yoshikazu@jp.fujitsu.com>,
        Amit Langote <amitlangote09@gmail.com>,
        Alvaro Herrera <alvherre@2ndquadrant.com>,
        Pg Hackers <pgsql-hackers@postgresql.org>
Subject: Re: speeding up planning with partitions
In-reply-to: <b8a139b8-2f5d-6079-67b2-99957af3fc36@lab.ntt.co.jp>
References: <8ea8bbb1-31ff-a469-02a7-281c3eba4bea@lab.ntt.co.jp>
 <201902011001.4v25uw6w6kb5@alvherre.pgsql>
 <CAKJS1f9LkQ=w4cq2_OaW05=mw8H_jQofWHiK0w3VVP-nSkUUmA@mail.gmail.com>
 <CA+HiwqFioAonMAgE0OWZeK=NEEOKmq-c=iYR2pkPOO=q6NxMkg@mail.gmail.com>
 <e85c4b41-80d6-1045-d934-feeca4a463ab@lab.ntt.co.jp>
 <0F97FA9ABBDBE54F91744A9B37151A5127865E@g01jpexmbkw24>
 <5d00eb35-375d-0672-0626-96bdb86829d0@lab.ntt.co.jp>
 <CAKJS1f9XAePd2+8F4=wtHKdDdckgKcSoXb-ZbU8Wbt86mhLxKA@mail.gmail.com>
 <b8a139b8-2f5d-6079-67b2-99957af3fc36@lab.ntt.co.jp>
Comments: In-reply-to Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
	message dated "Tue, 12 Feb 2019 15:41:15 +0900"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <30971.1550510855.1@sss.pgh.pa.us>
Date: Mon, 18 Feb 2019 12:27:35 -0500
Message-ID: <30972.1550510855@sss.pgh.pa.us>
Precedence: bulk

Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
> [ v22 patch set ]

I started to look at this, and immediately choked on the 0001 patch:

        if (childpruned ||
            !apply_child_basequals(root, rel, childrel, childRTE, appinfo) ||
            relation_excluded_by_constraints(root, childrel, childRTE))
        {

Frankly, that code is just horrid.  Having a function with side effects
in an if-test is questionable at the best of times, and having it be
the second of three conditions (which the third condition silently depends
on) is unreadable and unmaintainable.

I think the existing code here is considerably cleaner than what this
patch proposes.

I suppose you are doing this because you intend to jam some additional
cleanup code into the successfully-pruned-it code path, but if said
code is really too bulky to have multiple copies of, couldn't you
put it into a subroutine?  You're not going to be able to get to only
one copy of such cleanup anyhow, because there is another early-exit
further down, for the case where set_rel_size detects dummy-ness.

			regards, tom lane