Re: speeding up planning with partitions

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp>
Cc: David Rowley <david(dot)rowley(at)2ndquadrant(dot)com>, "Imai, Yoshikazu" <imai(dot)yoshikazu(at)jp(dot)fujitsu(dot)com>, Amit Langote <amitlangote09(at)gmail(dot)com>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: speeding up planning with partitions
Date: 2019-02-18 17:27:35
Message-ID: 30972.1550510855@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2019-02-18 17:37:27 Re: 2019-03 CF Summary / Review - Tranche #2
Previous Message Stephen Frost 2019-02-18 17:12:36 Re: WAL insert delay settings