Hi, On 2019-06-18 00:00:28 -0700, Andres Freund wrote: > On 2019-06-13 16:23:34 -0700, Andres Freund wrote: > > On June 13, 2019 3:38:47 PM PDT, Tom Lane <t...@sss.pgh.pa.us> wrote: > > >Andres Freund <and...@anarazel.de> writes: > > >> I am too tired to look further into this. I suspect the only reason > > >we > > >> didn't previously run into trouble with the executor stashing > > >hashkeys > > >> manually at a different tree level with: > > >> ((HashState *) innerPlanState(hjstate))->hashkeys > > >> is that hashkeys itself isn't printed... > > > > > >TBH, I think 5f32b29c is just wrong and should be reverted for now. > > >If there's a need to handle those expressions differently, it will > > >require some cooperation from the planner not merely a two-line hack > > >in executor startup. That commit didn't include any test case or > > >other demonstration that it was solving a live problem, so I think > > >we can leave it for v13 to address the issue. > > > > I'm pretty sure you'd get an assertion failure if you reverted it > > (that's why it was added). So it's a bit more complicated than that. > > Unfortunately I'll not get back to work until Monday, but I'll spend > > time on this then. > > Indeed, there are assertion failures when initializing the expression > with HashJoinState as parent - that's because when computing the > hashvalue for nodeHash input, we expect the slot from the node below to > be of the type that HashState returns (as that's what INNER_VAR for an > expression at the HashJoin level refers to), rather than the type of the > input to HashState. We could work around that by marking the slots from > underlying nodes as being of an unknown type, but that'd slow down > execution. > > I briefly played with the dirty hack of set_deparse_planstate() > setting dpns->inner_planstate = ps for IsA(ps, HashState), but that > seems just too ugly. > > I think the most straight-forward fix might just be to just properly > split the expression at plan time. Adding workarounds for things as > dirty as building an expression for a subsidiary node in the parent, and > then modifying the subsidiary node from the parent, doesn't seem like a > better way forward. > > The attached *prototype* does so. > > If we go that way, we probably need to: > - Add a test for the failure case at hand > - check a few of the comments around inner/outer in nodeHash.c > - consider moving the setrefs.c code into its own function? > - probably clean up the naming scheme in createplan.c > > I think there's a few more things we could do, although it's not clear > that that needs to happen in v12: > - Consider not extracting hj_OuterHashKeys, hj_HashOperators, > hj_Collations out of HashJoin->hashclauses, and instead just directly > handing them individually in the planner. create_mergejoin_plan() > already partially does that.
Tom, any comments? Otherwise I'll go ahead, and commit after a round or two of polishing. - Andres
