Re: Fix bogus Asserts in calc_non_nestloop_required_outer

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Sat, Jan 6, 2024 at 4:08 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> The argument for the patch as proposed is that we should make the
>> mergejoin and hashjoin code paths do what the nestloop path is doing.
>> However, as I replied further down in that other thread, I'm not
>> exactly convinced that the nestloop path is doing the right thing.
>> I've not had time to look closer at that, though.

> I don't really understand what you were saying in your response there,
> or what you're saying here. It seems to me that if the path is
> parameterized by top relids, and the assertion is verifying that a
> certain set of non-toprelids i.e. otherrels isn't present, then
> obviously the assertion is going to pass, but it's not checking what
> it purports to be checking.

I think we're talking at cross-purposes. What I was wondering about
(at least further down in the thread) was whether we shouldn't be
checking *both* the "real" and the "parent" relids to make sure they
don't overlap the parameterization sets. But it's probably overkill.
After reading the code again I agree that the parent relids are more
useful to check.

However, I still don't like Richard's patch too much as-is, because
the Asserts are difficult to read/understand and even more difficult
to compare to the other code path. I think we want to keep the
nestloop and not-nestloop paths as visually similar as possible,
so I propose we do it more like the attached (where I also borrowed
some of your wording for the comments).

A variant of this could be to ifdef out all the added code in
non-Assert builds:

Relids outer_paramrels = PATH_REQ_OUTER(outer_path);
Relids inner_paramrels = PATH_REQ_OUTER(inner_path);
Relids required_outer;
#ifdef USE_ASSERT_CHECKING
Relids innerrelids;
Relids outerrelids;

/*
* Any parameterization of the input paths refers to topmost parents of
* the relevant relations, because reparameterize_path_by_child() hasn't
* been called yet. So we must consider topmost parents of the relations
* being joined, too, while checking for disallowed parameterization
* cases.
*/
if (inner_path->parent->top_parent_relids)
innerrelids = inner_path->parent->top_parent_relids;
else
innerrelids = inner_path->parent->relids;

if (outer_path->parent->top_parent_relids)
outerrelids = outer_path->parent->top_parent_relids;
else
outerrelids = outer_path->parent->relids;

/* neither path can require rels from the other */
Assert(!bms_overlap(outer_paramrels, innerrelids));
Assert(!bms_overlap(inner_paramrels, outerrelids));
#endif
/* form the union ... */

but I'm not sure that's better. Probably any reasonable compiler
will throw away the whole calculation upon noticing the outputs
are unused.

regards, tom lane