Re: [HACKERS] Rethinking behavior of force_parallel_mode = regress

[Robert Haas]

On Tue, Jun 21, 2016 at 4:41 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Sat, Jun 18, 2016 at 4:49 PM, Tom Lane  wrote:
>>> With that thought in mind, I propose that the behavior of
>>> force_parallel_mode = regress is ill-designed so far as EXPLAIN is
>>> concerned.  What it ought to do is suppress *all* Gathers from the output,
>>> not just ones that were added in response to force_parallel_mode itself.
>
>> No, that doesn't sound like a very good idea.  If you do that, then
>> you have no hope of the differences being *zero*, because any place
>> that the regression tests are intended to produce a parallel plan is
>> going to look different.
>
> Well, sure, but in those areas you just set force_parallel_mode to on.

Well, I don't see how that gets you anywhere.  Now every regression
test that generates a parallel plan needs a decoration to set
force_parallel_mode=on temporarily and then change it back to regress
afterwards.  And once you've done that, you no longer get any benefit
out of having changed the behavior of force_parallel_mode=regress.
Either I need more caffeine, or this is a bad plan.  Possibly both,
because I definitely need more caffeine.

>> The charter of force_parallel_mode=regress
>> is that any regression test that passes normally should still pass
>> with that setting.
>
> I would like that charter to include scenarios with other nondefault GUC
> settings, to the extent we can reasonably make it work, because setting
> *only* force_parallel_mode is pretty sad in terms of test coverage.
> Or hadn't you noticed all the bugs we flushed from cover as soon as we
> tried changing the cost values?

Well, I did send a WIP patch to set consider_parallel correctly for
upper rels, which helps a lot, but you seem not to have looked at it.
I think we should fix the bugs in the current approach before deciding
that it doesn't work.  That having been said, I can't disagree with
the principle you're setting forth here.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Rethinking behavior of force_parallel_mode = regress

[Tom Lane]

Robert Haas  writes:
> On Sat, Jun 18, 2016 at 4:49 PM, Tom Lane  wrote:
>> With that thought in mind, I propose that the behavior of
>> force_parallel_mode = regress is ill-designed so far as EXPLAIN is
>> concerned.  What it ought to do is suppress *all* Gathers from the output,
>> not just ones that were added in response to force_parallel_mode itself.

> No, that doesn't sound like a very good idea.  If you do that, then
> you have no hope of the differences being *zero*, because any place
> that the regression tests are intended to produce a parallel plan is
> going to look different.

Well, sure, but in those areas you just set force_parallel_mode to on.

> The charter of force_parallel_mode=regress
> is that any regression test that passes normally should still pass
> with that setting.

I would like that charter to include scenarios with other nondefault GUC
settings, to the extent we can reasonably make it work, because setting
*only* force_parallel_mode is pretty sad in terms of test coverage.
Or hadn't you noticed all the bugs we flushed from cover as soon as we
tried changing the cost values?

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Rethinking behavior of force_parallel_mode = regress

[Robert Haas]

On Sat, Jun 18, 2016 at 4:49 PM, Tom Lane  wrote:
> As of HEAD it is possible to get through all of our regression tests
> with these settings:
>
> alter system set force_parallel_mode = regress;
> alter system set max_parallel_workers_per_gather = 2;
> alter system set parallel_tuple_cost = 0;
> alter system set parallel_setup_cost = 0;
> alter system set min_parallel_relation_size = 0;
>
> although there are quite a number of cosmetic differences in the outputs
> for the core regression tests.  (Curiously, contrib, pl, and isolation
> seem to pass without any diffs.)  In view of the number of bugs we've been
> able to identify with this setup, it would be nice to reduce the volume of
> the cosmetic differences to make it easier to review the diffs by hand.
> I'm not sure there's much that can be done about the row-ordering diffs;
> some randomness in the output order from a parallel seqscan seems
> inevitable.  But we could tamp down the EXPLAIN output differences, which
> are much harder to review anyway.
>
> With that thought in mind, I propose that the behavior of
> force_parallel_mode = regress is ill-designed so far as EXPLAIN is
> concerned.  What it ought to do is suppress *all* Gathers from the output,
> not just ones that were added in response to force_parallel_mode itself.

No, that doesn't sound like a very good idea.  If you do that, then
you have no hope of the differences being *zero*, because any place
that the regression tests are intended to produce a parallel plan is
going to look different.  The charter of force_parallel_mode=regress
is that any regression test that passes normally should still pass
with that setting.  This change would clearly break that.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers