Обсуждение: are the 2 if-statements in join_is_legal() removable?
Hi:
In join_is_legal(), there are 2 decision-making statements based on match_sjinfo. I wonder wether their conditions can ever test possitive.
/*
* If one input contains min_lefthand and the other contains
* min_righthand, then we can perform the SJ at this join.
*
* Reject if we get matches to more than one SJ; that implies we're
* considering something that's not really valid.
*/
if (bms_is_subset(sjinfo->min_lefthand, rel1->relids) &&
bms_is_subset(sjinfo->min_righthand, rel2->relids))
{
if (match_sjinfo)
return false; /* invalid join path */
match_sjinfo = sjinfo;
reversed = false;
}
else if (bms_is_subset(sjinfo->min_lefthand, rel2->relids) &&
bms_is_subset(sjinfo->min_righthand, rel1->relids))
{
if (match_sjinfo)
return false; /* invalid join path */
match_sjinfo = sjinfo;
reversed = true;
}
There is no query in regression test suite that can render the 2 decision-makings based on match_sjinfo true, nor can i figure out one. Can these conditions ever be true? If they can be true, what queries can make them true? On the contrary,if the conditions can never be true, then the 2 "if (match_sjinfo) return false" statements can be safely removed.
Any feedback is welcome.
--
Best Regards
Geng
g l <orangegrove@live.com> writes: > In join_is_legal(), there are 2 decision-making statements based on match_sjinfo. I wonder wether their conditions canever test possitive. The code coverage report says yes. https://coverage.postgresql.org/src/backend/optimizer/path/joinrels.c.gcov.html#350 regards, tom lane
Hi Tom:
Thanks for your reply! I add logs like this:
Thanks for your reply! I add logs like this:
if (bms_is_subset(sjinfo->min_lefthand, rel1->relids) &&
bms_is_subset(sjinfo->min_righthand, rel2->relids))
{
if (match_sjinfo) {
elog(LOG, "gunkris111111111111");
return false; /* invalid join path */
}
match_sjinfo = sjinfo;
reversed = false;
}
else if (bms_is_subset(sjinfo->min_lefthand, rel2->relids) &&
bms_is_subset(sjinfo->min_righthand, rel1->relids))
{
if (match_sjinfo) {
elog(LOG, "gunkris222222222222");
return false; /* invalid join path */
}
match_sjinfo = sjinfo;
reversed = true;
}
else if (sjinfo->jointype == JOIN_SEMI &&
bms_equal(sjinfo->syn_righthand, rel2->relids) &&
create_unique_path(root, rel2, rel2->cheapest_total_path,
sjinfo) != NULL)
{
if (match_sjinfo) {
elog(LOG, "gunkris33333333333333");
return false; /* invalid join path */
}
match_sjinfo = sjinfo;
reversed = false;
unique_ified = true;
}
else if (sjinfo->jointype == JOIN_SEMI &&
bms_equal(sjinfo->syn_righthand, rel1->relids) &&
create_unique_path(root, rel1, rel1->cheapest_total_path,
sjinfo) != NULL)
{
/* Reversed semijoin case */
if (match_sjinfo) {
elog(LOG, "gunkris4444444444444444");
return false; /* invalid join path */
}
match_sjinfo = sjinfo;
reversed = true;
unique_ified = true;
}
Then I run the core regression tests with "make installcheck-parallel
" for v18beta1 as the document dictates. When the regression test is done, only the last 2 log entries are found in logfile, the first 2 entries are not printed at all. I run core regression also with v14.15 and v17.2, the results are the same. What test suite do I need to run to cover the first 2 branches, or could you please show me a example query that can cover them? Thank you very much.
发件人: Tom Lane <tgl@sss.pgh.pa.us>
发送时间: 2025年5月10日 18:47
收件人: g l <orangegrove@live.com>
抄送: pgsql-general@postgresql.org <pgsql-general@postgresql.org>
主题: Re: are the 2 if-statements in join_is_legal() removable?
发送时间: 2025年5月10日 18:47
收件人: g l <orangegrove@live.com>
抄送: pgsql-general@postgresql.org <pgsql-general@postgresql.org>
主题: Re: are the 2 if-statements in join_is_legal() removable?
g l <orangegrove@live.com> writes:
> In join_is_legal(), there are 2 decision-making statements based on match_sjinfo. I wonder wether their conditions can ever test possitive.
The code coverage report says yes.
https://coverage.postgresql.org/src/backend/optimizer/path/joinrels.c.gcov.html#350
regards, tom lane
> In join_is_legal(), there are 2 decision-making statements based on match_sjinfo. I wonder wether their conditions can ever test possitive.
The code coverage report says yes.
https://coverage.postgresql.org/src/backend/optimizer/path/joinrels.c.gcov.html#350
regards, tom lane
g l <orangegrove@live.com> writes:
> Then I run the core regression tests with "make installcheck-parallel
> " for v18beta1 as the document dictates. When the regression test is done, only the last 2 log entries are found in
logfile,the first 2 entries are not printed at all. I run core regression also with v14.15 and v17.2, the results are
thesame. What test suite do I need to run to cover the first 2 branches, or could you please show me a example query
thatcan cover them? Thank you very much.
The coverage.postgresql.org results are, I believe, for check-world
not just the core tests. You might try contrib/postgres_fdw first,
as that tends to be the non-core test with the most planner coverage.
In any case, the fact that a few of these lines are not reached by
test cases doesn't constitute a bug, and it most certainly doesn't
mean it'd be safe to remove them. Most of the logic in join_is_legal
comes in symmetrical pairs of cases for the two possible orderings
of the two input relations. It's just coincidental which of a pair
of cases will be exercised by a given phrasing of a SQL query.
So I don't feel that we're particularly short on coverage here:
one or the other code path is fully exercised in each case,
according to the coverage.postgresql.org results.
regards, tom lane
Hi Tom:
Yes, there are many symmetrical branches in join_is_legal(). For a pair of symetrical branches, it is ok if any of them is covered.
In the following pair of symetrical branches, there are 2 'return false' statements, one in each branch. At least one of them should be covered.
if (bms_is_subset(sjinfo->min_lefthand, rel1->relids) &&
bms_is_subset(sjinfo->min_righthand, rel2->relids))
{
if (match_sjinfo)
return false; /* invalid join path */
match_sjinfo = sjinfo;
reversed = false;
}
else if (bms_is_subset(sjinfo->min_lefthand, rel2->relids) &&
bms_is_subset(sjinfo->min_righthand, rel1->relids))
{
if (match_sjinfo)
return false; /* invalid join path */
match_sjinfo = sjinfo;
reversed = true;
}
I run the check-world test, adding log entries as follows to observe coverage status:
if (match_sjinfo) {
int fd=open("/logfile", O_RDWR | O_APPEND);
write(fd, "kkkk111\n", 8);
close(fd);
return false; /* invalid join path */
}
But still, none of the 2 'return false' statement is covered according to log output. It is not due to logfile write failure, because i also added log entries exactly the same way at other points, and those log entries can be seen in logfile after the test was done. I also noticed from screen output that there are 17 TAP test scripts skipped, due to reasons like "Potentially unsafe test oauth not enabled in PG_TEST_EXTRA", "Injection points not supported by this build", "test xid_wraparound not enabled in PG_TEST_EXTRA" or "ICU not supported by this build". I examined the skipped scripts, none of these scripts have keywords like 'left join', 'right join', 'full join'. Some of them have queries with exists-sublink or any-sublink, but none of such queries will have more than 1 sjinfo after sublink/subquery pull-up. Since a query with less than 2 sjinfo cannot cover any of the 2 'return false' statements in question, I believe i didn't miss out on any covering query due to the skipped scripts. Maybe the coverage report you sent to me that shows the 1st 'return false' statement gets covered is generated from some 'bigger' test suite not included in the v18beta source tarball? My purpose is to understand why the decision making statements 'if (match_sjinfo) return false' is necessary here and in which circumstance will its condition test true, so a query that can make 'if (match_sjinfo)' test true is sufficient. Could you please show me an example query? Thank you very much!
best regards, geng
发件人: Tom Lane <tgl@sss.pgh.pa.us>
发送时间: 2025年5月11日 18:31
收件人: g l <orangegrove@live.com>
抄送: pgsql-general@postgresql.org <pgsql-general@postgresql.org>
主题: Re: 回复: are the 2 if-statements in join_is_legal() removable?
发送时间: 2025年5月11日 18:31
收件人: g l <orangegrove@live.com>
抄送: pgsql-general@postgresql.org <pgsql-general@postgresql.org>
主题: Re: 回复: are the 2 if-statements in join_is_legal() removable?
g l <orangegrove@live.com> writes:
> Then I run the core regression tests with "make installcheck-parallel
> " for v18beta1 as the document dictates. When the regression test is done, only the last 2 log entries are found in logfile, the first 2 entries are not printed at all. I run core regression also with v14.15 and v17.2, the results are the same. What test suite do I need to run to cover the first 2 branches, or could you please show me a example query that can cover them? Thank you very much.
The coverage.postgresql.org results are, I believe, for check-world
not just the core tests. You might try contrib/postgres_fdw first,
as that tends to be the non-core test with the most planner coverage.
In any case, the fact that a few of these lines are not reached by
test cases doesn't constitute a bug, and it most certainly doesn't
mean it'd be safe to remove them. Most of the logic in join_is_legal
comes in symmetrical pairs of cases for the two possible orderings
of the two input relations. It's just coincidental which of a pair
of cases will be exercised by a given phrasing of a SQL query.
So I don't feel that we're particularly short on coverage here:
one or the other code path is fully exercised in each case,
according to the coverage.postgresql.org results.
regards, tom lane
> Then I run the core regression tests with "make installcheck-parallel
> " for v18beta1 as the document dictates. When the regression test is done, only the last 2 log entries are found in logfile, the first 2 entries are not printed at all. I run core regression also with v14.15 and v17.2, the results are the same. What test suite do I need to run to cover the first 2 branches, or could you please show me a example query that can cover them? Thank you very much.
The coverage.postgresql.org results are, I believe, for check-world
not just the core tests. You might try contrib/postgres_fdw first,
as that tends to be the non-core test with the most planner coverage.
In any case, the fact that a few of these lines are not reached by
test cases doesn't constitute a bug, and it most certainly doesn't
mean it'd be safe to remove them. Most of the logic in join_is_legal
comes in symmetrical pairs of cases for the two possible orderings
of the two input relations. It's just coincidental which of a pair
of cases will be exercised by a given phrasing of a SQL query.
So I don't feel that we're particularly short on coverage here:
one or the other code path is fully exercised in each case,
according to the coverage.postgresql.org results.
regards, tom lane