Re: BUG #16536: Segfault with partition-wise joins

PG Bug reporting form <noreply(at)postgresql(dot)org> writes:
> The problem seems to be that in conditions of a bitmap index scan under a
> bitmapAnd, in the case of a partitionwise join, a Var isn't being replaced
> properly by a Param, resulting in a crash when we attempt to validate the
> expression since no slot is available for the Var. The bitmapAnd seems to be
> required to trip the bug, with a single bitmap index scan it does not
> manifest.

I believe I see the problem. create_bitmap_and_path and
create_bitmap_or_path figure that they can cheat and not
mark the path with parameterization info:

pathnode->path.param_info = NULL; /* not used in bitmap trees */

However, reparameterize_path_by_child thinks it only has to
recurse into sub-paths that have relevant parameterization:

/*
* If the path is not parameterized by parent of the given relation, it
* doesn't need reparameterization.
*/
if (!path->param_info ||
!bms_overlap(PATH_REQ_OUTER(path), child_rel->top_parent_relids))
return path;

So, if we try to reparameterize a parameterized bitmap path,
nothing happens to the children of BitmapAnd/Or sub-paths,
and we end up with a broken path leading to a busted plan.

It's a bit embarrassing that this has escaped detection.
I see from coverage.postgresql.org that we have zero coverage
for bitmap paths here, as well as several other path types,
which now one must wonder if those cases work either.
(It's not real clear to me if this code can be reached in
cases other than enable_partitionwise_join; the effects might
be pretty limited if not.)

Anyway, I can see a couple of routes to a fix:

(1) Change create_bitmap_and_path and create_bitmap_or_path
to account for parameterization honestly. This is certainly
the cleanest fix, but it would add some cycles, and what's
probably a bigger issue for back-patching is that their
signatures would have to change. Maybe that's okay?
There's probably not a reason for external code to call them,
and codesearch.debian.net knows of no instances of that.

(2) Hack up reparameterize_path_by_child so that it will descend
into these nodes regardless of their parameterization markers.
That's okay from an efficiency standpoint, since we'd already
have stopped at the parent BitmapHeapPath if it weren't
parameterized. But it seems quite ugly.

Another point I notice is that reparameterize_path thinks it
doesn't need to touch sub-structure at all when increasing
the parameterization of a BitmapHeapPath. Maybe that's okay
but it probably deserves more thought, especially since I see
that the case is again untested.

Thoughts?

regards, tom lane