Subject: Re: Declarative partitioning - another take
References: <ad16e2f5-fc7c-cc2d-333a-88d4aa446f96@lab.ntt.co.jp>
 <CA+TgmoZS+Pnk-J_+MZ96Z6vHsAwzSmF0vPeBmXoqhjY4H=23Fw@mail.gmail.com>
 <169708f6-6e5a-18d1-707b-1b323e4a6baf@lab.ntt.co.jp>
 <CAFjFpRcR=8J0kSx6kHVWGtun1k+Jfb3JfV0R038ns0a_287HAg@mail.gmail.com>
 <4f7108cb-7185-e956-d746-ad756b52431a@lab.ntt.co.jp>
 <CAKcux6=6teuMLy6ogqqOBTHck5GEs+wnL2-Gm1Y2TfXzGtijWQ@mail.gmail.com>
 <CA+HiwqFrnJWHeC7GqHa_dZDpidzvv3cfXwC0_hmYYg7UQOL=3Q@mail.gmail.com>
 <28ee345c-1278-700e-39a7-36a71f9a3b43@lab.ntt.co.jp>
 <e5c1c9cf-3f5a-c4d7-6047-7351147aaef9@lab.ntt.co.jp>
 <CA+Tgmobs7C+4t5xm9LGopUb=2HF+SBE2EQ2NRH5EMd+drs6OUQ@mail.gmail.com>
 <7aa6a14d-9476-afd7-2c09-6a016eb8c12b@lab.ntt.co.jp>
 <CA+TgmoaPxXJ14eDVia514UiuQAXyZGqfbz8Qg3G4a8Rz2gKF7w@mail.gmail.com>
 <377bc541-2108-4500-4b87-1b5936553555@lab.ntt.co.jp>
 <CA+TgmoYJcUTcN7vVgg54GHtffH11JJWYZnfF4KiRxjV-iaACQg@mail.gmail.com>
 <9ac4ba01-35cf-658b-5a57-c516c4ccedff@lab.ntt.co.jp>
 <CA+TgmobMy=rqM=MTN_FUEfD-PiWSCSonH+Z1_SjL6ZmQ2GGz1w@mail.gmail.com>
From: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
Message-ID: <0a299fbf-e5a1-dc3d-eb5e-11d3601eac16@lab.ntt.co.jp>
Date: Wed, 2 Nov 2016 11:55:51 +0900
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101
 Thunderbird/45.4.0
MIME-Version: 1.0
In-Reply-To: 
 <CA+TgmobMy=rqM=MTN_FUEfD-PiWSCSonH+Z1_SjL6ZmQ2GGz1w@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit
To: Robert Haas <robertmhaas@gmail.com>
Cc: Amit Langote <amitlangote09@gmail.com>,
        Rajkumar Raghuwanshi <rajkumar.raghuwanshi@enterprisedb.com>,
        Ashutosh Bapat <ashutosh.bapat@enterprisedb.com>,
        Pg Hackers <pgsql-hackers@postgresql.org>
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org

On 2016/11/02 2:53, Robert Haas wrote:
> On Fri, Oct 28, 2016 at 3:53 AM, Amit Langote
> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>>  [ new patches ]
> 
> Reviewing 0005:
> 
> Your proposed commit messages says this:
> 
>> If relation is the target table (UPDATE and DELETE), flattening is
>> done regardless (scared to modify inheritance_planner() yet).

I should have explained my thinking behind why I ended up not handling the
result relation case:

While set_append_rel_size() can be safely invoked recursively on the roots
of partition sub-trees, I was not quite sure if inheritance_planner() is
amenable to such recursive invocation.  While the former is relatively
straightforward baserel processing, the latter is not-so-straightforward
transformation of the whole query for every target child relation of the
target parent.

> In the immortal words of Frank Herbert: “I must not fear. Fear is the
> mind-killer. Fear is the little-death that brings total obliteration.
> I will face my fear. I will permit it to pass over me and through me.
> And when it has gone past I will turn the inner eye to see its path.
> Where the fear has gone there will be nothing. Only I will remain.”
> 
> In other words, I'm not going to accept fear as a justification for
> randomly-excluding the target-table case from this code.  If there's
> an actual reason not to do this in that case or some other case, then
> let's document that reason.  But weird warts in the code that are
> justified only by nameless anxiety are not good.

Perhaps the above comment expands a little on the actual reason.  Though I
guess it still amounts to giving up on doing a full analysis of whether
recursive processing within inheritance_planner() is feasible.

I think we could just skip this patch as long as a full investigation into
inheritance_planner() issues is not done.  It's possible that there might
be other places in the planner code which are not amenable to the proposed
multi-level AppendRelInfos.  Ashutosh had reported one [1], wherein
lateral joins wouldn't work with multi-level partitioned table owing to
the multi-level AppendRelInfos (the patch contains a hunk to address that).

> Of course, the prior question is whether we should EVER be doing this.
> I realize that something like this MAY be needed for partition-wise
> join, but the mission of this patch is not to implement partition-wise
> join.  Does anything in this patch series really require this?  If so,
> what?  If not, how about we leave it out and refactor it when that
> change is really needed for something?

Nothing *requires* it as such.  One benefit I see is that exclusion of the
root of some partition sub-tree means the whole sub-tree is excluded in
one go.  Currently, because of the flattening, every relation in that
sub-tree would be excluded separately, needlessly repeating the expensive
constraint exclusion proof. But I guess that's just an optimization.

Thanks,
Amit

[1]
https://www.postgresql.org/message-id/CAFjFpReZF34MDbY95xoATi0xVj2mAry4-LHBWVBayOc8gj%3Diqg%40mail.gmail.com


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