RE: speeding up planning with partitions

From: "Imai, Yoshikazu" <imai(dot)yoshikazu(at)jp(dot)fujitsu(dot)com>
To: 'Tom Lane' <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp>
Cc: David Rowley <david(dot)rowley(at)2ndquadrant(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-19 00:49:28
Message-ID: 0F97FA9ABBDBE54F91744A9B37151A5127E7D2@g01jpexmbkw24
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Mon, Feb 18, 2019 at 5:28 PM, Tom Lane wrote:
> 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.

When I reviewed this, I thought there are no problems in the codes, but I googled what Tom pointed out[1], read it and I was ashamed of my ignorance.

> 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?

ISTM the 0004 patch eventually removes these codes from multiple places (set_append_rel_size and set_inherited_target_rel_sizes) so we might be better to not be struggling here?

[1] https://www.teamten.com/lawrence/programming/keep-if-clauses-side-effect-free.html

--
Yoshikazu Imai

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Amit Langote 2019-02-19 00:54:47 Re: speeding up planning with partitions
Previous Message Andres Freund 2019-02-19 00:41:07 Re: Speed up transaction completion faster after many relations are accessed in a transaction