MIME-Version: 1.0
In-Reply-To: <5358.1533241519@sss.pgh.pa.us>
References: <5AFC0865.8050802@lab.ntt.co.jp> <5AFD6580.5090308@lab.ntt.co.jp>
 <5AFE81CC.8000508@lab.ntt.co.jp> <5B17C5EC.1090602@lab.ntt.co.jp>
 <CAFjFpRffA-BfL0RXcUVLLbcOBF1ie7oObTB-RE2q_o45X1HTYg@mail.gmail.com>
 <5B1E5071.1000704@lab.ntt.co.jp> <5B23A964.9020800@lab.ntt.co.jp>
 <5B28FB19.3090809@lab.ntt.co.jp>
 <CA+TgmoZboTszGQk3OecBOZUFNNjzKE-SOo_Q46xtTJ4Yj95-RQ@mail.gmail.com>
 <20180703163519.tsigyic5xefunv74@alap3.anarazel.de>
 <5B3C2A4E.5030901@lab.ntt.co.jp>
 <5B4F2BE9.3000304@lab.ntt.co.jp>
 <CA+TgmobR9XWEhmF3s_WsoLNnnjaFyS_xtbrVG3dCGiUgof20Nw@mail.gmail.com>
 <CA+TgmoZSaKq-fYALn5jf6c_X3==Rb2s8eqLDwGpV=LNNhTXYwg@mail.gmail.com>
 <5B55886F.5090906@lab.ntt.co.jp>
 <CA+TgmoYYTCQL=TUYbUz2b=CsQeB4o1L2+_=zHynxGVmyKUtH1A@mail.gmail.com>
 <5B5712C8.9030404@lab.ntt.co.jp>
 <CA+TgmoazFzdp5ZVwUjk_b2cuB90WCeeWtzt1ShSbXGyg67c+Zw@mail.gmail.com>
 <5B59BA55.30200@lab.ntt.co.jp> <5B619D23.4010000@lab.ntt.co.jp>
 <CA+TgmoZMdNiKUGV0=-QLV1f1-Xr0CDT+8y9wLktuEiQGkt64UQ@mail.gmail.com>
 <CA+TgmoZjYHtkw-o_4=-GXb6obF1epyivr+WnZ1SQCnU6fgtf+w@mail.gmail.com>
 <5358.1533241519@sss.pgh.pa.us>
From: Robert Haas <robertmhaas@gmail.com>
Date: Fri, 3 Aug 2018 09:44:52 -0400
Message-ID: 
 <CA+TgmoYJ_HmMjGr-y2fg71XQ6J3S9ro6oehJmYkkoGVwZSwe7w@mail.gmail.com>
Subject: Re: Expression errors with "FOR UPDATE" and postgres_fdw with
 partition wise join enabled.
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>,
 Andres Freund <andres@anarazel.de>,
	Ashutosh Bapat <ashutosh.bapat@enterprisedb.com>,
	Rajkumar Raghuwanshi <rajkumar.raghuwanshi@enterprisedb.com>,
	pgsql-hackers <pgsql-hackers@postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Aug 2, 2018 at 4:25 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I do not trust Ashutosh's patch because of the point you noted that it
> will kick in on ConvertRowtypeExprs that are not related to partitioning.

I don't remember making that point -- can you clarify?

> It's also got a rather fundamental conceptual (or at least documentation)
> error in that it says it's making pull_var_clause do certain things to
> all ConvertRowtypeExprs, but that's not what the code actually does.
> I think the tension around that has a lot to do with the fact that the
> patch went through so many revisions, and I don't have any faith that
> it's right even yet.  As you mentioned upthread, this might be insoluble
> without some representational change for converted whole-row Vars.

Insoluble is a strong word....

> What I'm thinking might be a more appropriate thing, at least for
> getting v11 out the door, is to refuse to generate partitionwise
> joins when any whole-row vars are involved.  (Perhaps that's not
> enough to dodge all the problems, though?)

It's enough to dodge the problem being discussed on this thread.

> Now, that's a bit of a problem for postgres_fdw, because it seems to
> insist on injecting WRVs even when the query text does not require any.
> Why is that, and can't we get rid of it?  It's horrid for performance
> even without any of these other considerations.  But if we fail to get
> rid of that in time for v11, it would mean that postgres_fdw can't
> participate in PWJ, which isn't great but I think we could live with it
> for now.

I don't quite know what you mean here -- postgres_fdw does use
whole-row vars for EPQ handling, which may be what you're thinking
about.

> BTW, what exactly do we think the production status of PWJ is, anyway?
> I notice that upthread it was stated that enable_partitionwise_join
> defaults to off, as indeed it still does, and we'd turn it on later
> when we'd gotten rid of some memory-hogging problems.  If that hasn't
> happened yet (and I don't see any open item about considering enabling
> PWJ by default for v11), then I have exactly no hesitation about
> lobotomizing PWJ as hard as we need to to mask this problem for v11.
> I'd certainly prefer a solution along those lines to either of these
> patches.

Yeah, that hasn't been done yet.  Partition-wise join probably needs a
good bit of work in a number of areas to do all the things that people
will want it to do -- see the thread on advanced partition-matching,
which is an improvement but still doesn't cover every case that could
come up.  In terms of memory usage, I think that we need some
discussion of the approach that should be taken.  I see a couple of
possible things we could do, not necessarily mutually exclusive.

1. We could try to avoid translating all of the parent's data
structures for every child RelOptInfo, either by rejiggering things so
that the child doesn't need all of that data, or by making it able to
use the parent's copy of the data.

2. We could try to recycle memory more quickly.  For example, if we're
planning a partition-wise join of A-B-C-D, first plan paths for
A1-B1-C1-D1 (and all proper subsets of those rels), then throw away
most of the memory, then plan paths for A2-B2-C2-D2, etc.

3. We could generate paths for on "representative" children (e.g. the
biggest ones) and use that to estimate the cost of the partition-wise
plan.  If that plan is selected, then go back and figure out real
paths for the other children.

All of these things help in different ways, and Ashutosh had code for
some version of all of them at various points, but the problem is
quite difficult.  If you have three tables with 1000 partitions each,
then without partition-wise join you need 3000 (partitions) + 3
(baserels) + 3 (level-2 joinrels) + 1 (level-3 joinrel) RelOptInfos.
If you do a partition-wise join, then you get 3000 level-2
child-joinrels and 1000 level-3 child joinrels.  Those RelOptInfo
structures, and the paths attached to them, cannot help but take up
memory.  Perhaps that's OK, and we ought to just say "well, if you
want better plans, you're going to have to allow more memory for
planning".  If not, we have to decide how hard we want to work in
which areas and how good the result needs to be in order to have this
turned on by difficult.

Honestly, I'm pretty impressed that we have added not one but two
members to the RelOptKind enum without as little collateral damage as
there has been.  This issue is annoying and the discussion has gone on
longer than anyone would probably prefer, but it's really pretty minor
in the grand scheme of things, at least IMHO.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company