[sqlsmith] Failed to generate plan on lateral subqueries
Hi,
I've added new grammar rules to sqlsmith and improved some older ones.
This was rewarded with a return of "failed to generate plan" errors.
The failing queries all contain a lateral subquery. The shortest of the
failing queries are below. They were run against the regression db of
master as of db07236.
regards,
Andreas
smith=# select msg, query from error where
(firstline(msg) ~~ 'ERROR: failed to build any%'
or firstline(msg) ~~ 'ERROR: could not devise a query plan%')
and t > now() - interval '1 day' order by length(query) asc limit 3;
ERROR: failed to build any 8-way joins
select
ref_96.foreign_table_schema as c0,
sample_87.is_supported as c1
from
information_schema.sql_packages as sample_87 tablesample system (0.2)
right join information_schema._pg_foreign_tables as ref_96
on (sample_87.feature_id = ref_96.foreign_table_catalog ),
lateral (select
sample_87.is_verified_by as c0,
ref_97.indexed_col as c1,
coalesce(sample_87.feature_id, ref_96.foreign_server_name) as c2,
4 as c3
from
public.comment_test as ref_97
where ref_97.id ~>~ ref_97.indexed_col
fetch first 73 rows only) as subq_33
where ref_96.foreign_table_name ~~ subq_33.c1
ERROR: could not devise a query plan for the given query
select
subq_43.c0 as c0
from
(select
ref_181.installed as c0
from
pg_catalog.pg_available_extension_versions as ref_181,
lateral (select
ref_181.name as c0,
ref_181.installed as c1
from
pg_catalog.pg_conversion as ref_182
where ref_182.conname ~~* ref_181.version
fetch first 98 rows only) as subq_42
where (subq_42.c0 is not NULL)
or (subq_42.c1 is NULL)) as subq_43
right join pg_catalog.pg_language as sample_177 tablesample system (2.8)
on (subq_43.c0 = sample_177.lanispl )
where sample_177.lanowner < sample_177.lanvalidator
ERROR: failed to build any 5-way joins
select
ref_239.id2 as c0,
40 as c1,
ref_239.id2 as c2,
ref_238.aa as c3
from
public.tt5 as sample_289 tablesample system (8.1)
inner join information_schema.element_types as ref_237
on (sample_289.x = ref_237.character_maximum_length )
left join public.b as ref_238
on (ref_237.character_maximum_length = ref_238.aa )
left join public.num_exp_mul as ref_239
on (ref_237.numeric_precision_radix = ref_239.id1 ),
lateral (select
sample_290.b as c0,
sample_289.y as c1,
ref_239.id2 as c2
from
public.rtest_t8 as sample_290 tablesample bernoulli (4.6)
where (sample_290.b > ref_238.bb)
and (sample_289.y > ref_239.expected)
fetch first 91 rows only) as subq_64
where (subq_64.c1 > sample_289.y)
and (sample_289.y = ref_239.expected)
fetch first 133 rows only
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2015/12/07 2:52, Andreas Seltenreich wrote:
Hi,
I've added new grammar rules to sqlsmith and improved some older ones.
This was rewarded with a return of "failed to generate plan" errors.
The failing queries all contain a lateral subquery. The shortest of the
failing queries are below. They were run against the regression db of
master as of db07236.smith=# select msg, query from error where
(firstline(msg) ~~ 'ERROR: failed to build any%'
or firstline(msg) ~~ 'ERROR: could not devise a query plan%')
and t > now() - interval '1 day' order by length(query) asc limit 3;ERROR: failed to build any 8-way joins
select
ref_96.foreign_table_schema as c0,
sample_87.is_supported as c1
from
information_schema.sql_packages as sample_87 tablesample system (0.2)
right join information_schema._pg_foreign_tables as ref_96
on (sample_87.feature_id = ref_96.foreign_table_catalog ),
lateral (select
sample_87.is_verified_by as c0,
ref_97.indexed_col as c1,
coalesce(sample_87.feature_id, ref_96.foreign_server_name) as c2,
4 as c3
from
public.comment_test as ref_97
where ref_97.id ~>~ ref_97.indexed_col
fetch first 73 rows only) as subq_33
where ref_96.foreign_table_name ~~ subq_33.c1ERROR: could not devise a query plan for the given query
select
subq_43.c0 as c0
from
(select
ref_181.installed as c0
from
pg_catalog.pg_available_extension_versions as ref_181,
lateral (select
ref_181.name as c0,
ref_181.installed as c1
from
pg_catalog.pg_conversion as ref_182
where ref_182.conname ~~* ref_181.version
fetch first 98 rows only) as subq_42
where (subq_42.c0 is not NULL)
or (subq_42.c1 is NULL)) as subq_43
right join pg_catalog.pg_language as sample_177 tablesample system (2.8)
on (subq_43.c0 = sample_177.lanispl )
where sample_177.lanowner < sample_177.lanvalidatorERROR: failed to build any 5-way joins
select
ref_239.id2 as c0,
40 as c1,
ref_239.id2 as c2,
ref_238.aa as c3
from
public.tt5 as sample_289 tablesample system (8.1)
inner join information_schema.element_types as ref_237
on (sample_289.x = ref_237.character_maximum_length )
left join public.b as ref_238
on (ref_237.character_maximum_length = ref_238.aa )
left join public.num_exp_mul as ref_239
on (ref_237.numeric_precision_radix = ref_239.id1 ),
lateral (select
sample_290.b as c0,
sample_289.y as c1,
ref_239.id2 as c2
from
public.rtest_t8 as sample_290 tablesample bernoulli (4.6)
where (sample_290.b > ref_238.bb)
and (sample_289.y > ref_239.expected)
fetch first 91 rows only) as subq_64
where (subq_64.c1 > sample_289.y)
and (sample_289.y = ref_239.expected)
fetch first 133 rows only
FWIW, I'm seeing the following behaviors:
* Removing the limit (fetch first...) in lateral sub-queries makes the
errors go away for all above queries.
* For the last query producing "failed to build any 5-way joins" error,
setting join_collapse_limit to 1, 2 and 4 makes the error go away
irrespective of whether the limit is kept or not.
Thanks,
Amit
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
On 2015/12/07 2:52, Andreas Seltenreich wrote:
I've added new grammar rules to sqlsmith and improved some older ones.
This was rewarded with a return of "failed to generate plan" errors.
The failing queries all contain a lateral subquery.
* Removing the limit (fetch first...) in lateral sub-queries makes the
errors go away for all above queries.
Yeah. I've been able to reduce Andreas' first example to
select * from
text_tbl tt1
left join int8_tbl i8 on i8.q1 = 42,
lateral (select
i8.q2,
tt2.f1
from
text_tbl tt2
limit 1
) as ss
where tt1.f1 = ss.f1;
ERROR: failed to build any 3-way joins
The key features are (1) a subquery with a LATERAL reference to the inner
side of a left join, and (2) a degenerate join condition for the left
join, ie it only references the inner side. (2) causes the planner to
see the left join as a clauseless join, so it prefers to postpone it
as long as possible. In this case it will try to join tt1 to ss first,
because the WHERE condition is an inner-join clause linking those two,
and inner joins are allowed to associate into the lefthand sides of left
joins. But now it's stuck: because of the lateral reference to i8, the
only way to generate a join between tt1+ss and i8 is for i8 to be on the
outside of a nestloop, and that doesn't work because nestloop can only
handle LEFT outer joins not RIGHT outer joins. So that's a dead end;
but because it thought earlier that tt1 could be joined to ss, it did not
generate the tt1+i8 join at all, so it fails to find any way to build the
final join.
If you remove the LIMIT then the sub-select can be flattened, causing
the problem to go away because there's no longer a lateral ordering
constraint (there's not actually any need to evaluate i8.q2 while
scanning tt2).
I think the way to fix this is that join_is_legal() should be taught to
notice whether the proposed join would have unresolved lateral references
to other relations that it will need to be on the outside of any join to.
If join_is_legal were to reject tt1+ss then the has_legal_joinclause
tests at the bottom of have_join_order_restriction would fail, so
have_join_order_restriction would correctly report that there's a
constraint forcing tt1 and i8 to be joined directly despite the lack
of a join clause.
Andreas' second example is a similar situation, with the addition of a
PlaceHolderVar in the sub-select (since it has a non-strict output
variable that has to bubble up through an outer join). In this case
we fail earlier, because although join_is_legal again lets us try to
make a join that can't be useful, the check_hazardous_phv() test that
I recently added to joinpath.c recognizes that there's no safe way
to make that join, so it rejects all the possible join paths and we
end up with a joinrel with no paths, leading to the different error
message.
I think that fixing join_is_legal() may be enough to take care of
this case too, but I need to think about whether there could still be
any cases in which check_hazardous_phv() would reject all paths for
a joinrel. It might be that that logic is in the wrong place and
needs to be folded into join_is_legal(), or that join_is_legal()
*also* has to account for this (which would be annoying).
I've not traced through the third example in detail, but it looks
like it's just a variant of these problems.
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
Andreas Seltenreich <seltenreich@gmx.de> writes:
I've added new grammar rules to sqlsmith and improved some older ones.
This was rewarded with a return of "failed to generate plan" errors.
I believe I've dealt with these cases now. Thanks for the report!
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
Tom Lane writes:
Andreas Seltenreich <seltenreich@gmx.de> writes:
I've added new grammar rules to sqlsmith and improved some older ones.
This was rewarded with a return of "failed to generate plan" errors.I believe I've dealt with these cases now. Thanks for the report!
I no longer see "failed to build any n-way joins" after pulling, but
there are still instances of "could not devise a query plan". Samples below.
regards,
Andreas
select
ref_1.aa as c0,
subq_1.c1 as c1,
coalesce(ref_1.class, ref_1.class) as c2,
subq_1.c0 as c3
from
(select
subq_0.c1 as c0,
coalesce(sample_0.a, sample_1.i) as c1
from
public.rtest_t9 as sample_0 tablesample bernoulli (5.6)
inner join public.iportaltest as sample_1 tablesample bernoulli (9.8)
on (sample_0.a = sample_1.i ),
lateral (select
sample_1.d as c0,
ref_0.a as c1,
sample_1.p as c2,
ref_0.a as c3,
ref_0.a as c4,
sample_0.b as c5,
sample_1.i as c6
from
public.rtest_view2 as ref_0
where sample_0.b = sample_0.b
fetch first 93 rows only) as subq_0
where sample_0.b ~<=~ sample_0.b) as subq_1
right join public.e_star as ref_1
on (subq_1.c0 = ref_1.aa )
where ref_1.cc < ref_1.cc
fetch first 59 rows only;
select
sample_69.tmpllibrary as c0,
coalesce(sample_69.tmplname, sample_69.tmplname) as c1,
subq_33.c0 as c2
from
(select
coalesce(ref_53.provider, sample_68.typdefault) as c0
from
pg_catalog.pg_type as sample_68 tablesample bernoulli (6.9)
inner join pg_catalog.pg_shseclabel as ref_53
on (sample_68.typowner = ref_53.objoid ),
lateral (select
sample_68.typcategory as c0,
ref_54.speaker as c1,
ref_54.speaker as c2
from
public.test_range_excl as ref_54
where (ref_53.label >= ref_53.provider)
and (ref_53.label !~* ref_53.provider)
fetch first 143 rows only) as subq_32
where ref_53.label ~>~ ref_53.label) as subq_33
right join pg_catalog.pg_pltemplate as sample_69 tablesample bernoulli (9.8)
on (subq_33.c0 = sample_69.tmplhandler )
where sample_69.tmplvalidator ~ subq_33.c0
fetch first 131 rows only;
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
I wrote:
Tom Lane writes:
Andreas Seltenreich <seltenreich@gmx.de> writes:
I've added new grammar rules to sqlsmith and improved some older ones.
This was rewarded with a return of "failed to generate plan" errors.I believe I've dealt with these cases now. Thanks for the report!
I no longer see "failed to build any n-way joins" after pulling, but
there are still instances of "could not devise a query plan". Samples below.
sorry, I spoke too soon: nine of the former have been logged through the
night. I'm attaching a larger set of sample queries this time in case
that there are still multiple causes for the observed errors.
regards,
Andreas
Andreas Seltenreich <seltenreich@gmx.de> writes:
I no longer see "failed to build any n-way joins" after pulling, but
there are still instances of "could not devise a query plan". Samples below.
sorry, I spoke too soon: nine of the former have been logged through the
night. I'm attaching a larger set of sample queries this time in case
that there are still multiple causes for the observed errors.
Hm. At least in the first of these cases, the problem is that the code
I committed yesterday doesn't account for indirect lateral dependencies.
That is, if S1 depends on S2 which depends on the inner side of an outer
join, it now knows not to join S2 directly to the outer side of the outer
join, but it doesn't realize that the same must apply to S1.
Maybe we should redefine lateral_relids as the transitive closure of
a rel's lateral dependencies? Not sure.
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, Dec 08, 2015 at 12:13:41PM -0500, Tom Lane wrote:
Andreas Seltenreich <seltenreich@gmx.de> writes:
I no longer see "failed to build any n-way joins" after pulling,
but there are still instances of "could not devise a query plan".
Samples below.sorry, I spoke too soon: nine of the former have been logged
through the night. I'm attaching a larger set of sample queries
this time in case that there are still multiple causes for the
observed errors.Hm. At least in the first of these cases, the problem is that the
code I committed yesterday doesn't account for indirect lateral
dependencies. That is, if S1 depends on S2 which depends on the
inner side of an outer join, it now knows not to join S2 directly to
the outer side of the outer join, but it doesn't realize that the
same must apply to S1.Maybe we should redefine lateral_relids as the transitive closure of
a rel's lateral dependencies? Not sure.
That seems like it would fix the problem once and for good. Is there
any reason to believe that the lateral dependencies could go in a
loop, i.e. is there a reason to put in checks for same in the
transitive closure construction?
Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.com
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
David Fetter <david@fetter.org> writes:
On Tue, Dec 08, 2015 at 12:13:41PM -0500, Tom Lane wrote:
Hm. At least in the first of these cases, the problem is that the
code I committed yesterday doesn't account for indirect lateral
dependencies. That is, if S1 depends on S2 which depends on the
inner side of an outer join, it now knows not to join S2 directly to
the outer side of the outer join, but it doesn't realize that the
same must apply to S1.Maybe we should redefine lateral_relids as the transitive closure of
a rel's lateral dependencies? Not sure.
That seems like it would fix the problem once and for good. Is there
any reason to believe that the lateral dependencies could go in a
loop, i.e. is there a reason to put in checks for same in the
transitive closure construction?
It should not be possible to have a loop, since rels can only have lateral
references to rels that appeared syntactically before them. But an Assert
about it doesn't seem a bad thing.
After working on this for awhile, I've found that indeed converting
lateral_relids to a transitive closure makes things better. But there was
another bug (or two other bugs, depending on how you count) exposed by
Andreas' examples. I was right after all to suspect that the "hazardous
PHV" condition needs to be accounted for by join_is_legal; as things
stood, join_is_legal might allow a join for which every possible path
would be rejected by check_hazardous_phv. More, if we were insisting that
a join PHV be calculated before we could pass it to some other relation,
we didn't actually do anything to ensure that that join would get built.
Given an appropriate set of conditions, the clauseless-join heuristics
could decide that that join wasn't interesting and never build it, again
possibly leading to planner failure. So join_is_legal's cousins
have_join_order_restriction and has_join_restriction also need to know
about this issue.
(This is a bit of a mess; I'm beginning to wonder if we shouldn't bite
the bullet and teach the executor how to handle non-Var NestLoopParams,
so that the planner doesn't have to work around that. But I'd rather
not back-patch such a change.)
I also noticed that lateral_referencers should be a transitive closure
as well. I think that oversight doesn't lead to any observable bug,
but it would lead to constructing index paths that cannot be used, so
fixing it should save some planner cycles.
So that leads to the attached patch, which I think is good as-is for
the back branches. In this state of the code, the LateralJoinInfo
list is darn near useless: the only thing we use it for is detecting
whether two relations have a direct (not transitive closure) lateral
reference. I'm strongly tempted to replace that logic by keeping a
copy of the pre-closure lateral_relids sets, and then we could drop
LateralJoinInfo altogether, which would make create_lateral_join_info
quite a bit shorter and faster. That could go into HEAD/9.5, but
I'm afraid to back-patch it further than 9.5 for fear that third-party
code somewhere might be relying on the LateralJoinInfo data structure.
Comments?
regards, tom lane
Attachments:
transitive-lateral-fixes-1.patchtext/x-diff; charset=us-ascii; name=transitive-lateral-fixes-1.patchDownload+483-337
On Wed, Dec 9, 2015 at 4:18 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
David Fetter <david@fetter.org> writes:
On Tue, Dec 08, 2015 at 12:13:41PM -0500, Tom Lane wrote:
Hm. At least in the first of these cases, the problem is that the
code I committed yesterday doesn't account for indirect lateral
dependencies. That is, if S1 depends on S2 which depends on the
inner side of an outer join, it now knows not to join S2 directly to
the outer side of the outer join, but it doesn't realize that the
same must apply to S1.Maybe we should redefine lateral_relids as the transitive closure of
a rel's lateral dependencies? Not sure.That seems like it would fix the problem once and for good. Is there
any reason to believe that the lateral dependencies could go in a
loop, i.e. is there a reason to put in checks for same in the
transitive closure construction?It should not be possible to have a loop, since rels can only have lateral
references to rels that appeared syntactically before them. But an Assert
about it doesn't seem a bad thing.After working on this for awhile, I've found that indeed converting
lateral_relids to a transitive closure makes things better. But there was
another bug (or two other bugs, depending on how you count) exposed by
Andreas' examples. I was right after all to suspect that the "hazardous
PHV" condition needs to be accounted for by join_is_legal; as things
stood, join_is_legal might allow a join for which every possible path
would be rejected by check_hazardous_phv. More, if we were insisting that
a join PHV be calculated before we could pass it to some other relation,
we didn't actually do anything to ensure that that join would get built.
Given an appropriate set of conditions, the clauseless-join heuristics
could decide that that join wasn't interesting and never build it, again
possibly leading to planner failure. So join_is_legal's cousins
have_join_order_restriction and has_join_restriction also need to know
about this issue.(This is a bit of a mess; I'm beginning to wonder if we shouldn't bite
the bullet and teach the executor how to handle non-Var NestLoopParams,
so that the planner doesn't have to work around that. But I'd rather
not back-patch such a change.)I also noticed that lateral_referencers should be a transitive closure
as well. I think that oversight doesn't lead to any observable bug,
but it would lead to constructing index paths that cannot be used, so
fixing it should save some planner cycles.So that leads to the attached patch, which I think is good as-is for
the back branches. In this state of the code, the LateralJoinInfo
list is darn near useless: the only thing we use it for is detecting
whether two relations have a direct (not transitive closure) lateral
reference. I'm strongly tempted to replace that logic by keeping a
copy of the pre-closure lateral_relids sets, and then we could drop
LateralJoinInfo altogether, which would make create_lateral_join_info
quite a bit shorter and faster. That could go into HEAD/9.5, but
I'm afraid to back-patch it further than 9.5 for fear that third-party
code somewhere might be relying on the LateralJoinInfo data structure.
Aside from the functional issues, could your changes result in
performance regressions? (if so, I'd argue not to backpatch unless
there were cases that returned bad data as opposed to spurious
errors).
merlin
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Merlin Moncure <mmoncure@gmail.com> writes:
Aside from the functional issues, could your changes result in
performance regressions? (if so, I'd argue not to backpatch unless
there were cases that returned bad data as opposed to spurious
errors).
I can't say that I think planner failures on valid queries is something
that's optional to fix. However, I believe that this will typically
not change the selected plan in cases where the planner didn't fail
before. I did notice one change in an existing regression test, where
the planner pushed a qual clause further down in the plan than it did
before; but that seems like a better plan anyway. (The reason that
happened is that the changes to enlarge the minimum parameterization
of some base rels result in choosing to push qualifiers further down,
since a qual clause will be evaluated at the lowest plan level that
the selected parameterization allows.)
It's a little bit harder to gauge the impact on planner speed. The
transitive closure calculation could be expensive in a query with many
lateral references, but that doesn't seem likely to be common; and anyway
we'll buy back some of that cost due to simpler tests later. I'm
optimistic that we'll come out ahead in HEAD/9.5 after the removal
of LateralJoinInfo setup. It might be roughly a wash in the back
branches.
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
Tom Lane writes:
[2. transitive-lateral-fixes-1.patch]
I was about to write that sqlsmith likes the patch, but after more than
10^8 ok queries the attached ones were generated.
regards,
Andreas
Attachments:
On Sun, Dec 6, 2015 at 9:52 AM, Andreas Seltenreich <seltenreich@gmx.de> wrote:
I've added new grammar rules to sqlsmith and improved some older ones.
Could you possibly teach sqlsmith about INSERT ... ON CONFLICT DO
UPDATE/IGNORE? I think that that could be very helpful, especially if
it could be done in advance of any stable release of 9.5.
Thanks
--
Peter Geoghegan
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Tom Lane writes:
Merlin Moncure <mmoncure@gmail.com> writes:
Aside from the functional issues, could your changes result in
performance regressions?
[...]
It's a little bit harder to gauge the impact on planner speed. The
transitive closure calculation could be expensive in a query with many
lateral references, but that doesn't seem likely to be common; and anyway
we'll buy back some of that cost due to simpler tests later. I'm
optimistic that we'll come out ahead in HEAD/9.5 after the removal
of LateralJoinInfo setup. It might be roughly a wash in the back
branches.
On the empirical side: I see a speedup of 0.4% in testing speed with the
patch applied. It could very well be me venting the room one additional
time during the second session, resulting in the CPUs spending more time
in their opportunistic frequency range or something.
regards,
Andreas
smith=# select extract('day' from t),
avg(generated/extract(epoch from updated - t)) as tps
from instance natural join stat
where generated > 1000000
and hostname not in('dwagon','slugbug')
-- these do stuff beside sqlsmith
and t > now() - interval '3 days' group by 1 order by 1;
date_part | tps
-----------+------------------
8 | 55.494181110456
9 | 55.6902316869404
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Dec 10, 2015 at 4:55 AM, Andreas Seltenreich <seltenreich@gmx.de> wrote:
Tom Lane writes:
Merlin Moncure <mmoncure@gmail.com> writes:
Aside from the functional issues, could your changes result in
performance regressions?[...]
It's a little bit harder to gauge the impact on planner speed. The
transitive closure calculation could be expensive in a query with many
lateral references, but that doesn't seem likely to be common; and anyway
we'll buy back some of that cost due to simpler tests later. I'm
optimistic that we'll come out ahead in HEAD/9.5 after the removal
of LateralJoinInfo setup. It might be roughly a wash in the back
branches.On the empirical side: I see a speedup of 0.4% in testing speed with the
patch applied. It could very well be me venting the room one additional
time during the second session, resulting in the CPUs spending more time
in their opportunistic frequency range or something.
Really...wow. That satisfies me. You ought to be commended for
sqlsmith -- great stuff.
merlin
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Andreas Seltenreich <seltenreich@gmx.de> writes:
Tom Lane writes:
[2. transitive-lateral-fixes-1.patch]
I was about to write that sqlsmith likes the patch, but after more than
10^8 ok queries the attached ones were generated.
Ah-hah --- the new check I added in join_is_legal understood about
chains of LATERAL references, but it forgot that we could also have chains
of outer-join ordering constraints. When we're looking to see if joining
on the basis of a LATERAL reference would break some later outer join, we
have to look at outer joins to the outer joins' inner relations, too.
Fixed in the attached. I also stuck all of join_is_legal's
lateral-related checks inside an "if (root->hasLateralRTEs)" block,
which will save some time in typical queries with no LATERAL. That
makes that section of the patch a bit bigger than before, but it's
mostly just reindentation.
Many thanks for the work you've done with this tool! Who knows how
long it would've taken us to find these problems otherwise ...
regards, tom lane
Attachments:
transitive-lateral-fixes-2.patchtext/x-diff; charset=us-ascii; name=transitive-lateral-fixes-2.patchDownload+597-377
I wrote:
Ah-hah --- the new check I added in join_is_legal understood about
chains of LATERAL references, but it forgot that we could also have chains
of outer-join ordering constraints. When we're looking to see if joining
on the basis of a LATERAL reference would break some later outer join, we
have to look at outer joins to the outer joins' inner relations, too.
Fixed in the attached. I also stuck all of join_is_legal's
lateral-related checks inside an "if (root->hasLateralRTEs)" block,
which will save some time in typical queries with no LATERAL. That
makes that section of the patch a bit bigger than before, but it's
mostly just reindentation.
As threatened, here's a patch on top of that that gets rid of
LateralJoinInfo. I'm pretty happy with this, although I have an
itchy feeling that we could dispense with the lateral_vars lists too.
regards, tom lane
Attachments:
remove-lateraljoininfo-1.patchtext/x-diff; charset=us-ascii; name=remove-lateraljoininfo-1.patchDownload+195-307
I wrote:
As threatened, here's a patch on top of that that gets rid of
LateralJoinInfo. I'm pretty happy with this, although I have an
itchy feeling that we could dispense with the lateral_vars lists too.
I experimented with that and figured out what was bothering me: there is
no need for add_placeholders_to_base_rels to fool around with adding items
to lateral_vars anymore, because the code in create_lateral_join_info
that adds placeholders' ph_lateral bits to the evaluation locations'
lateral_relids sets now does everything that we needed to have happen.
We don't care exactly what's inside the PHV expression, only which rels
it comes from, and the ph_lateral bitmap is sufficient for that.
We still need the lateral_vars field in RelOptInfo, but it now exists
purely to carry the results of extract_lateral_references forward to
create_lateral_join_info. (We wouldn't need even that so far as Vars are
concerned, because we could just add their source rel to the referencing
rel's lateral_relids on-the-fly in extract_lateral_references. But we
can't handle PHVs that way, because at that stage we don't know precisely
where they'll be evaluated, so we don't know what to add to the
referencer's lateral_relids.)
Updated patch attached; compared to the previous one, this just removes a
big chunk of code in add_placeholders_to_base_rels and updates a couple of
related comments.
regards, tom lane
Attachments:
remove-lateraljoininfo-2.patchtext/x-diff; charset=us-ascii; name=remove-lateraljoininfo-2.patchDownload+250-362
Tom Lane writes:
[2. transitive-lateral-fixes-2.patch ]
[2. remove-lateraljoininfo-2.patch ]
They seem to have fixed the issue for good now. No errors have been
logged for 2e8 queries since applying the first patch. (The second one
was applied later and didn't get as much exposure.) I guess that means
I have to go back to extending the grammar again :-).
regards
Andreas
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Andreas Seltenreich <seltenreich@gmx.de> writes:
Tom Lane writes:
[2. transitive-lateral-fixes-2.patch ]
[2. remove-lateraljoininfo-2.patch ]
They seem to have fixed the issue for good now. No errors have been
logged for 2e8 queries since applying the first patch. (The second one
was applied later and didn't get as much exposure.)
Thanks. It's good that you tested both states of the code, since I intend
to back-patch transitive-lateral-fixes into 9.4 and 9.3, but not the
second patch.
I guess that means I have to go back to extending the grammar again :-).
I await the results with interest. Did you note the suggestion about
trying to stress the ON CONFLICT code with this? You'd need it to
issue non-SELECT queries, which might create some reproducibility
issues...
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