回复: 回复: are the 2 if-statements in join_is_legal() removable?

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(at)sss(dot)pgh(dot)pa(dot)us>
发送时间: 2025年5月11日 18:31
收件人: g l <orangegrove(at)live(dot)com>
抄送: pgsql-general(at)postgresql(dot)org <pgsql-general(at)postgresql(dot)org>
主题: Re: 回复: are the 2 if-statements in join_is_legal() removable?

g l <orangegrove(at)live(dot)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