Re: wrong query result with jit_above_cost= 0

> On Wed, 27 Jun 2018 at 17:49, Dmitry Dolgov <9erthalion6@gmail.com> wrote:
>
> > On 26 June 2018 at 22:56, Andres Freund <andres@anarazel.de> wrote:
> > On 2018-06-26 21:55:07 +0100, Andrew Gierth wrote:
> >> >>>>> "Dmitry" == Dmitry Dolgov <9erthalion6@gmail.com> writes:
> >>
> >>  Dmitry> Yep, my bad, forgot to turn it on. Now I see what's the
> >>  Dmitry> problem, one of the null fields is screwed up, will try to
> >>  Dmitry> figure out why is that.
> >>
> >> The handling of nulls in grouping set results is a bit icky, see
> >> prepare_projection_slot in nodeAgg.c. The comment there lists a number
> >> of assumptions which may or may not hold true under JIT which might give
> >> a starting point to look for problems. (Unfortunately I'm not currently
> >> in a position to test on a JIT build)
> >
> > I probably just screwed up a bit of code generation. I can't see any of
> > the more fundamental assumptions being changed by the way JITing is
> > done.
>
> So far I found out that in agg_retrieve_hash_table, when there is a scan for
> TupleHashEntryData, that contains AggStatePerGroup structure in the field
> "additional", it's possible to get some garbage data (or at least transValue is
> lost). It happens when we do:
>
> ReScanExprContext(aggstate->aggcontexts[i]);
>
> in agg_retrieve_direct before that. Apparently, the reason is that in the jit
> code there is a store operation for curaggcontext into aggcontext:
>
> v_aggcontext = l_ptr_const(op->d.agg_trans.aggcontext,
>                            l_ptr(StructExprContext));
>
> /* set aggstate globals */
> LLVMBuildStore(b, v_aggcontext, v_curaggcontext);
>
> I haven't found anything similar in the original code or in the other branches
> for aggregation logic. I can't say that I fully understand the idea behind it,
> but at least it was suspicious for me. When I removed this operation, the
> problem has disappeared.

Ok, looks like I found the issue. v_aggcontext & v_curaggcontext have nothing
to do here (this change was just masking a problem by changing a memory context
so that the wrong one will never be used). The problem was that in the
llvmjit_expr in AGG_INIT_TRANS section we need to assign a current memory
context from op->d.agg_init_trans.aggcontext (see the attached patch),
otherwise we'll get in the situation when current memory context is hashcontext
instead of aggcontext.

Also, I found one suspicious thing, in AGG_PLAIN_TRANS section we don't
switch the memory context back in the branch with ExecAggTransReparent. I
never found any consequences of that, but just in case I believe it makes sense
to do so.

And the last thing - where can I find a documentation about how to properly
apply patches for GDB & perf support to llvm? I remember they were posted here,
and found some of them here [1] from Andres, but apparently part of them was
already applied on top of llvm. Looks like for the gdb support I need to apply
0006-ORC-JIT-event-listener-support (since there is a gdb listener mentioned
there), but with this patch I have an error:

    error: ‘ObjHandleT’ was not declared in this scope

So I'm confused how should it be?

[1]: https://www.postgresql.org/message-id/20171005065739.dgsplipwkpmrkspg%40alap3.anarazel.de