Re: apply_scanjoin_target_to_paths and partitionwise join

On Fri, May 24, 2024 at 11:02 AM <arne(dot)roland(at)malkut(dot)net> wrote:

> Hi Ashutosh,
>
> thanks for bringing this to my attention. I'll first share a few
> thoughts about the change and respond regarding the test below.
>
> I clearly understand your intention with this patch. It's an issue I run
> into from time to time.
>
> I did some testing with some benchmark sets back with pg 14. I did the
> following: I planned with and without the partitionwise join GUC
> (explain) and took the one with the lower cost to execute the query.
>
> Interestingly, even discounting the overhead and additional planning
> time, the option with the lower cost turned out to be slower on our
> benchmark set back then. The median query with disabled GUC was quicker,
> but on average that was not the case. The observation is one, I'd
> generally describe as "The more options you consider, the more ways we
> have to be horribly wrong. More options for the planner are a great way
> to uncover the various shortcomings of it."
>
> That might be specific to the benchmark I was working with at the time.
> But that made me drop the issue back then. That is ofc no valid reason
> not to go in the direction of making the planner to consider more
> options. :)
>

In summary, you are suggesting that partitionwise join performs better than
plain join even if the latter one has lower cost. Hence fixing this issue
has never become a priority for you. Am I right?

Plans with lower costs being slower is not new for optimizer.
Partitionwise join just adds another case.

>
> Maybe we can discuss that in person next week?
>

Sure.

>
> On 2024-05-22 07:57, Ashutosh Bapat wrote:
> >
> > The test was added by 6b94e7a6da2f1c6df1a42efe64251f32a444d174 and
> > later modified by 3c569049b7b502bb4952483d19ce622ff0af5fd6. The
> > modification just avoided eliminating the join, so that change can be
> > ignored. 6b94e7a6da2f1c6df1a42efe64251f32a444d174 added the tests to
> > test fractional paths being considered when creating ordered append
> > paths. Reading the commit message, I was expecting a test which did
> > not use a join as well and also which used inheritance. But it seems
> > that the queries added by that commit, test all the required scenarios
> > and hence we see two queries involving join between partitioned
> > tables. As the comment there says the intention is to verify index
> > only scans and not exactly partitionwise join. So just fixing the
> > expected output of one query looks fine. The other query will test
> > partitionwise join and fractional paths anyway. I am including Tomas,
> > Arne and Zhihong, who worked on the first commit, to comment on
> > expected output changes.
>
> The test was put there to make sure a fractional join is considered in
> the case that a partitionwise join is considered. Because that wasn't
> the case before.
>
> The important part for my use case back then was that we do Merge
> Join(s) at all. The test result after your patch still confirms that.
>
> If we simply modify the test as such, we no longer confirm, whether the
> code path introduced in 6b94e7a6da2f1c6df1a42efe64251f32a444d174 is
> still working.
>
> Maybe it's worthwhile to add something like
>
> create index on fract_t0 ((id*id));
>
> EXPLAIN (COSTS OFF)
> SELECT * FROM fract_t x JOIN fract_t y USING (id) ORDER BY id * id DESC
> LIMIT 10;
> QUERY PLAN
>
> -------------------------------------------------------------------------------
> Limit
> -> Merge Append
> Sort Key: ((x.id * x.id)) DESC
> -> Nested Loop
> -> Index Scan Backward using fract_t0_expr_idx on
> fract_t0 x_1
> -> Index Only Scan using fract_t0_pkey on fract_t0 y_1
> Index Cond: (id = x_1.id)
> -> Sort
> Sort Key: ((x_2.id * x_2.id)) DESC
> -> Hash Join
> Hash Cond: (x_2.id = y_2.id)
> -> Seq Scan on fract_t1 x_2
> -> Hash
> -> Seq Scan on fract_t1 y_2
>
>
> I am not sure, whether it's worth the extra test cycles on every animal,
> but since we are not creating an extra table it might be ok.
> I don't have a very strong feeling about the above test case.
>

My patch removes redundant enable_partitionwise_join = on since that's done
very early in the test. Apart from that it does not change the test. So if
the expected output change is fine with you, I think we should leave the
test as is. Plan outputs are sometimes fragile and thus make expected
outputs flaky.

> > I will create patches for the back-branches once the patch for master
> > is in a committable state.
>
> I am not sure, whether it's really a bug. I personally wouldn't be brave
> enough to back patch this. I don't want to deal with complaining end
> users. Suddenly their optimizer, which always had horrible estimates,
> was actually able to do harmful stuff with them. Only due to a minor
> version upgrade. I think that's a bad idea to backpatch something with
> complex performance implications. Especially since they might even be
> based on potentially inaccurate data...
>

Since it's a thinko I considered it as a bug. But I agree that it has the
potential to disturb plans after upgrade and thus upset users. So I am fine
if we don't backpatch.

--
Best Wishes,
Ashutosh Bapat