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

Andrew Gierth <andrew(at)tao11(dot)riddles(dot)org(dot)uk> writes:
> "Tom" == Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
> Tom> Anyway, I can see a couple of routes to a fix:

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

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

> Well the obvious compromise fix is to do 2 in the back-branches and 1 in
> head, but that may be overkill...

I realized there's another way, which is to make create_bitmap_and_path
and create_bitmap_or_path compute the correct parameterization as the
union of the outer rels required by their child paths. Then they don't
need any additional arguments. I had been thinking of having indxpath.c
pass down the parameterization, but it turns out that that code isn't
tracking that anyway, so it would have largely had to do it like this
anyhow.

As a further benefit, now that BitmapAndPath and BitmapOrPath
aren't special in this way, we can get rid of indxpath.c's
get_bitmap_tree_required_outer() function; it's sufficient to do
PATH_REQ_OUTER() on the top-level path node in a bitmap tree.
So the attached patch actually nets out at less code than before.

You could argue I guess that although this isn't an ABI break, it's
still an API change: now these functions are required to set the
param_info field in their output correctly. That would only break
external code if it's building such Path structures by hand rather
than using these functions. I devoutly hope nobody is ... though
I did have to slap down some code in indxpath.c which was doing
exactly that.

This lacks regression test case(s) as yet, but I think it's
commitable otherwise. I'm going to try to build some test cases
that improve the coverage report in this area...

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

> Hmm. I'm not sure I fully understand the implications of what's going on
> there, but if new quals are effectively being moved into the path as a
> result of the reparameterization, then leaving the substructure alone
> would presumably mean that those new quals can only become Filter:
> clauses. But presumably, if they could be usefully indexed, then we
> would have already generated a parameterized path that included them?

We're not actually moving any new quals into the path, we're just changing
its labeling. Given the way I've done things here, I no longer think it's
a problem. I'd worried that we might need BitmapAnd/OrPaths to have
exactly the same required_outer as their parent BitmapHeapPath, but in
the event it should be fine when they have subset required_outer values.

regards, tom lane