pg.ddx.io  pgsql-hackers@postgresql.org mailing list archive  

help / color / mirror / Atom feed
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>
Cc: Imai, Yoshikazu <imai.yoshikazu@jp.fujitsu.com>
Cc: Amit Langote <amitlangote09@gmail.com>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: Pg Hackers <pgsql-hackers@postgresql.org>
Subject: Re: speeding up planning with partitions
Date: Mon, 18 Feb 2019 12:27:35 -0500
Message-ID: <30972.1550510855@sss.pgh.pa.us> (raw)
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>

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




view thread (265+ messages)  latest in thread

Message-ID: <30972.1550510855@sss.pgh.pa.us>
Permalink:  ../30972.1550510855@sss.pgh.pa.us/
Also on:    postgresql.org/message-id/30972.1550510855@sss.pgh.pa.us

 · 

reply
Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: pgsql-hackers@postgresql.org
  Cc: tgl@sss.pgh.pa.us, Langote_Amit_f8@lab.ntt.co.jp, david.rowley@2ndquadrant.com, imai.yoshikazu@jp.fujitsu.com, amitlangote09@gmail.com, alvherre@2ndquadrant.com
  Subject: Re: speeding up planning with partitions
  In-Reply-To: <30972.1550510855@sss.pgh.pa.us>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox