Subject: Re: Query with high planning time at version 11.1 compared versions
 10.5 and 11.0
References: <8db4eca8-6e76-70ff-f934-e0228a1cc298@lab.ntt.co.jp>
 <20181206075726.pig32e6thltb56hr@alvherre.pgsql>
 <CAExHW5umxU5FOoT8FBcqp-GWKMtZhnuEy7U6bNH8QfzvYwUgEQ@mail.gmail.com>
 <CAExHW5uFwP_1N7RV6s8piQBvygGH+rppS60nFNHB9DKEM04ShA@mail.gmail.com>
 <5C34846E.9090600@lab.ntt.co.jp>
 <96720c99-ffa0-01ad-c594-0504c8eda708@lab.ntt.co.jp>
 <5C35D912.8040200@lab.ntt.co.jp>
 <7354b5bf-5932-4c0c-81ee-d9550de18995@lab.ntt.co.jp>
 <5C36E105.2020902@lab.ntt.co.jp>
From: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
Message-ID: <dd1de042-dca5-cfb9-bc66-2e39020fbf6a@lab.ntt.co.jp>
Date: Fri, 11 Jan 2019 13:46:09 +0900
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:60.0) Gecko/20100101
 Thunderbird/60.3.3
MIME-Version: 1.0
In-Reply-To: <5C36E105.2020902@lab.ntt.co.jp>
Content-Type: multipart/mixed;
 boundary="------------0DE0BF1DD1E8E3C24C44CCF5"
Content-Language: en-US
To: Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
Cc: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>, alvherre@2ndquadrant.com,
        pryzby@telsasoft.com, sanyo.moura@tatic.net,
        Jeff Janes <jeff.janes@gmail.com>, david.rowley@2ndquadrant.com,
        Robert Haas <robertmhaas@gmail.com>, pgsql-hackers@postgresql.org
Precedence: bulk

This is a multi-part message in MIME format.
--------------0DE0BF1DD1E8E3C24C44CCF5
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit

Fujita-san,

On 2019/01/10 15:07, Etsuro Fujita wrote:
> Amit-san,
> 
> (2019/01/10 10:41), Amit Langote wrote:
>> On 2019/01/09 20:20, Etsuro Fujita wrote:
>>> I like your patch in general.  I think one way to address Ashutosh's
>>> concerns would be to use the consider_partitionwise_join flag: originally,
>>> that was introduced for partitioned relations to show that they can be
>>> partitionwise-joined, but I think that flag could also be used for
>>> non-partitioned relations to show that they have been set up properly for
>>> partitionwise-joins, and I think by checking that flag we could avoid
>>> creating those copies for child dummy rels in try_partitionwise_join.
>>
>> Ah, that's an interesting idea.
>>
>> If I understand the original design of it correctly,
>> consider_partitionwise_join being true for a given relation (simple or
>> join) means that its RelOptInfo contains properties to consider it to be
>> joined with another relation (simple or join) using partitionwise join
>> mechanism.  Partitionwise join will occur between the pair if the other
>> relation also has relevant properties (hence its
>> consider_partitionwise_join set to true) and properties on the two sides
>> match.
> 
> Actually, the flag being true just means that the tlist for a given
> partitioned relation (simple or join) doesn't contain any whole-row Vars. 
> And if two given partitioned relations having the flag being true have
> additional properties to be joined using the PWJ technique, then we try to
> do PWJ for those partitioned relations.

I see.  Thanks for the explanation.

> (The name of the flag isn't
> good?  If so, that would be my fault because I named that flag.)

If it's really just to store the fact that the relation's targetlist
contains expressions that partitionwise join currently cannot handle, then
setting it like this in set_append_rel_size seems a bit misleading:

    if (enable_partitionwise_join &&
        rel->reloptkind == RELOPT_BASEREL &&
        rte->relkind == RELKIND_PARTITIONED_TABLE &&
        rel->attr_needed[InvalidAttrNumber - rel->min_attr] == NULL)
        rel->consider_partitionwise_join = true;

Sorry, I wasn't around to comment on the patch which got committed in
7cfdc77023a, but checking the value of enable_partitionwise_join and other
things in set_append_rel_size() to set the value of
consider_partitionwise_join seems a bit odd to me.  Perhaps,
consider_partitionwise_join should be initially set to true for a relation
(actually, to rel->part_scheme != NULL) and only set it to false if the
relation's targetlist is found to contain unsupported expressions.  That
way, it becomes easier to think what it means imho.  I think
enable_partitionwise_join should only be checked in relnode.c or
joinrels.c.  I've attached a patch to show what I mean. Can you please
take a look?

If you think that this patch is a good idea, then you'll need to
explicitly set consider_partitionwise_join to false for a dummy partition
rel in set_append_rel_size(), because the assumption of your patch that
such partition's rel's consider_partitionwise_join would be false (as
initialized with the current code) would be broken by my patch.  But that
might be a good thing to do anyway as it will document the special case
usage of consider_partitionwise_join variable more explicitly, assuming
you'll be adding a comment describing why it's being set to false explicitly.

>> That's a loaded meaning and abusing it to mean something else can be
>> challenged, but we can live with that if properly documented.  Speaking of
>> which:
>>
>>      /* used by partitionwise joins: */
>>      bool        consider_partitionwise_join;    /* consider
>> partitionwise join
>>                                                   * paths? (if partitioned
>> rel) */
>>
>> Maybe, mention here how it will be abused in back-branches for
>> non-partitioned relations?
> 
> Will do.

Thank you.

>>> Please find attached an updated version of the patch.  I modified your
>>> version so that building tlists for child dummy rels are also postponed
>>> until after they actually participate in partitionwise-joins, to avoid
>>> that possibly-useless work as well.  I haven't done any performance tests
>>> yet though.
>>
>> Thanks for updating the patch.  I tested your patch (test setup described
>> below) and it has almost the same performance as my previous version:
>> 552ms (vs. 41159ms on HEAD vs. 253ms on PG 10) for the query also
>> mentioned below.
> 
> Thanks for that testing!
> 
> I also tested the patch with your script:
> 
> 253.559 ms (vs. 85776.515 ms on HEAD vs. 206.066 ms on PG 10)

Oh, PG 11 doesn't appear as bad compared to PG 10 with your numbers as it
did on my machine.  That's good anyway.

Thanks,
Amit

--------------0DE0BF1DD1E8E3C24C44CCF5
Content-Type: text/plain; charset=UTF-8;
 name="tweak-setting-consider_partitionwise_join.patch"
Content-Transfer-Encoding: base64
Content-Disposition: attachment;
 filename="tweak-setting-consider_partitionwise_join.patch"

ZGlmZiAtLWdpdCBhL3NyYy9iYWNrZW5kL29wdGltaXplci9wYXRoL2FsbHBhdGhzLmMgYi9z
cmMvYmFja2VuZC9vcHRpbWl6ZXIvcGF0aC9hbGxwYXRocy5jCmluZGV4IDE5OTljZDQ0MzYu
LjI0OTAwY2U1OTMgMTAwNjQ0Ci0tLSBhL3NyYy9iYWNrZW5kL29wdGltaXplci9wYXRoL2Fs
bHBhdGhzLmMKKysrIGIvc3JjL2JhY2tlbmQvb3B0aW1pemVyL3BhdGgvYWxscGF0aHMuYwpA
QCAtMTMwNSwxNSArMTMwNSwxMyBAQCBzZXRfYXBwZW5kX3JlbF9zaXplKFBsYW5uZXJJbmZv
ICpyb290LCBSZWxPcHRJbmZvICpyZWwsCiAJfQogCiAJLyoKLQkgKiBJZiB0aGlzIGlzIGEg
cGFydGl0aW9uZWQgYmFzZXJlbCwgc2V0IHRoZSBjb25zaWRlcl9wYXJ0aXRpb253aXNlX2pv
aW4KLQkgKiBmbGFnOyBjdXJyZW50bHksIHdlIG9ubHkgY29uc2lkZXIgcGFydGl0aW9ud2lz
ZSBqb2lucyB3aXRoIHRoZSBiYXNlcmVsCi0JICogaWYgaXRzIHRhcmdldGxpc3QgZG9lc24n
dCBjb250YWluIGEgd2hvbGUtcm93IFZhci4KKwkgKiBDdXJyZW50bHksIHdlIG9ubHkgY29u
c2lkZXIgcGFydGl0aW9ud2lzZSBqb2lucyB3aXRoIHRoZSBiYXNlcmVsCisJICogaWYgaXRz
IHRhcmdldGxpc3QgZG9lc24ndCBjb250YWluIGEgd2hvbGUtcm93IFZhcjsgdHVybiBpdCBv
ZmYKKwkgKiBvdGhlcndpc2UuCiAJICovCi0JaWYgKGVuYWJsZV9wYXJ0aXRpb253aXNlX2pv
aW4gJiYKLQkJcmVsLT5yZWxvcHRraW5kID09IFJFTE9QVF9CQVNFUkVMICYmCi0JCXJ0ZS0+
cmVsa2luZCA9PSBSRUxLSU5EX1BBUlRJVElPTkVEX1RBQkxFICYmCi0JCXJlbC0+YXR0cl9u
ZWVkZWRbSW52YWxpZEF0dHJOdW1iZXIgLSByZWwtPm1pbl9hdHRyXSA9PSBOVUxMKQotCQly
ZWwtPmNvbnNpZGVyX3BhcnRpdGlvbndpc2Vfam9pbiA9IHRydWU7CisJaWYgKHJlbC0+Y29u
c2lkZXJfcGFydGl0aW9ud2lzZV9qb2luICYmCisJCXJlbC0+YXR0cl9uZWVkZWRbSW52YWxp
ZEF0dHJOdW1iZXIgLSByZWwtPm1pbl9hdHRyXSAhPSBOVUxMKQorCQlyZWwtPmNvbnNpZGVy
X3BhcnRpdGlvbndpc2Vfam9pbiA9IGZhbHNlOwogCiAJLyoKIAkgKiBJbml0aWFsaXplIHRv
IGNvbXB1dGUgc2l6ZSBlc3RpbWF0ZXMgZm9yIHdob2xlIGFwcGVuZCByZWxhdGlvbi4KZGlm
ZiAtLWdpdCBhL3NyYy9iYWNrZW5kL29wdGltaXplci91dGlsL3JlbG5vZGUuYyBiL3NyYy9i
YWNrZW5kL29wdGltaXplci91dGlsL3JlbG5vZGUuYwppbmRleCBmZTgzZWM0NTE5Li44MGMy
NmFmNGExIDEwMDY0NAotLS0gYS9zcmMvYmFja2VuZC9vcHRpbWl6ZXIvdXRpbC9yZWxub2Rl
LmMKKysrIGIvc3JjL2JhY2tlbmQvb3B0aW1pemVyL3V0aWwvcmVsbm9kZS5jCkBAIC0yNTMs
NiArMjUzLDkgQEAgYnVpbGRfc2ltcGxlX3JlbChQbGFubmVySW5mbyAqcm9vdCwgaW50IHJl
bGlkLCBSZWxPcHRJbmZvICpwYXJlbnQpCiAJCQlicmVhazsKIAl9CiAKKwkvKiBTZXQgaWYg
dGhpcyByZWxhdGlvbiBjYW4gcGFydGljaXBhdGUgaW4gcGFydGl0aW9ud2lzZSBqb2luLiAq
LworCXJlbC0+Y29uc2lkZXJfcGFydGl0aW9ud2lzZV9qb2luID0gKHJlbC0+cGFydF9zY2hl
bWUgIT0gTlVMTCk7CisKIAkvKiBTYXZlIHRoZSBmaW5pc2hlZCBzdHJ1Y3QgaW4gdGhlIHF1
ZXJ5J3Mgc2ltcGxlX3JlbF9hcnJheSAqLwogCXJvb3QtPnNpbXBsZV9yZWxfYXJyYXlbcmVs
aWRdID0gcmVsOwogCg==
--------------0DE0BF1DD1E8E3C24C44CCF5--