Rethinking behavior of force_parallel_mode = regress
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.
I experimented with the attached prototype patch and found that it indeed
greatly reduces the volume of EXPLAIN differences, though it doesn't
remove them all. I did not for example try to hide the effects of partial
aggregation.
If we were to do this, we could remove the Gather.invisible plan node
field altogether, which would be good cleanup in my book.
Even if we don't do this, my code review showed that there's a bug in
what ExplainPrintPlan is doing right now for the case: it neglects to
run InstrEndLoop on the topmost node, which at the very least risks
confusing auto_explain.
Thoughts?
regards, tom lane
Attachments:
explain-suppress-all-gathers.patchtext/x-diff; charset=us-ascii; name=explain-suppress-all-gathers.patchDownload+40-16
On Sat, Jun 18, 2016 at 4:49 PM, Tom Lane <tgl@sss.pgh.pa.us> 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
Robert Haas <robertmhaas@gmail.com> writes:
On Sat, Jun 18, 2016 at 4:49 PM, Tom Lane <tgl@sss.pgh.pa.us> 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
On Tue, Jun 21, 2016 at 4:41 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.com> writes:
On Sat, Jun 18, 2016 at 4:49 PM, Tom Lane <tgl@sss.pgh.pa.us> 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