Re: PG 15 (and to a smaller degree 14) regression due to ExprEvalStep size - Mailing list pgsql-hackers
From | Andres Freund |
---|---|
Subject | Re: PG 15 (and to a smaller degree 14) regression due to ExprEvalStep size |
Date | |
Msg-id | 20220802003947.x2ezq7u7pkmxt6ie@awork3.anarazel.de Whole thread Raw |
In response to | Re: PG 15 (and to a smaller degree 14) regression due to ExprEvalStep size (Amit Langote <amitlangote09@gmail.com>) |
Responses |
Re: PG 15 (and to a smaller degree 14) regression due to ExprEvalStep size
|
List | pgsql-hackers |
Hi, On 2022-07-27 17:01:13 +0900, Amit Langote wrote: > Here's an updated version of the patch, with mostly cosmetic changes. > In particular, I added comments describing the new llvm_compile_expr() > blobs. - I've asked a couple times before: Why do we need space for every possible datatype at once in JsonItemCoercions? Can there be multiple "concurrent" coercions in process? The whole coercion stuff just seems incredibly clunky (in a slightly different shape before this patch). ExecEvalJsonExprItemCoercion() calls ExecPrepareJsonItemCoercion(), which gets a pointer to one of the per-type elements in JsonItemCoercionsState, dispatching on the type of the json object. Then we later call ExecGetJsonItemCoercion() (via a convoluted path), which again will dispatch on the type (extracting the json object again afaics!), to then somehow eventually get the coerced value. I cannot make any sense of this. This code should not have been committed in this state. - Looks like there's still some recursive expression states, namely JsonExprState->{result_coercion, coercions}? - Looks like the JsonExpr code in ExecInitExprRec() is big enough to potentially benefit from splitting out into a separate function? - looks like JsonExprPostEvalState could be moved to execExprInterp.c? - I ran the patch against LLVM 14 built with assertions enabled, and it triggers an assertion failure: #3 0x00007f75d165c242 in __GI___assert_fail ( assertion=0x7f75c278d511 "getOperand(0)->getType() == getOperand(1)->getType() && \"Both operands to ICmp instructionare not of the same type!\"", file=0x7f75c2780366 "/home/andres/src/llvm-project-14/llvm/include/llvm/IR/Instructions.h", line=1192, function=0x7f75c27d9dcc "void llvm::ICmpInst::AssertOK()") at assert.c:101 #4 0x00007f75c2b9b25c in llvm::ICmpInst::AssertOK (this=0x55e019290ca0) at /home/andres/src/llvm-project-14/llvm/include/llvm/IR/Instructions.h:1191 #5 0x00007f75c2b9b0ea in llvm::ICmpInst::ICmpInst (this=0x55e019290ca0, pred=llvm::CmpInst::ICMP_EQ, LHS=0x55e019290c10,RHS=0x55e01928ce80, NameStr="") at /home/andres/src/llvm-project-14/llvm/include/llvm/IR/Instructions.h:1246 #6 0x00007f75c2b93c99 in llvm::IRBuilderBase::CreateICmp (this=0x55e0192894f0, P=llvm::CmpInst::ICMP_EQ, LHS=0x55e019290c10,RHS=0x55e01928ce80, Name="") at /home/andres/src/llvm-project-14/llvm/include/llvm/IR/IRBuilder.h:2202 #7 0x00007f75c2c1bc5d in LLVMBuildICmp (B=0x55e0192894f0, Op=LLVMIntEQ, LHS=0x55e019290c10, RHS=0x55e01928ce80, Name=0x7f75d0d24cbc"") at /home/andres/src/llvm-project-14/llvm/lib/IR/Core.cpp:3927 #8 0x00007f75d0d20b1f in llvm_compile_expr (state=0x55e019201380) at /home/andres/src/postgresql/src/backend/jit/llvm/llvmjit_expr.c:2392 ... #19 0x000055e0184c16d4 in exec_simple_query (query_string=0x55e01912f6e0 "SELECT JSON_EXISTS(NULL::jsonb, '$');") at /home/andres/src/postgresql/src/backend/tcop/postgres.c:1204 this triggers easily interactively - which is nice because that allows to dump the types: p getOperand(0)->getType()->dump() -> prints i64 p getOperand(1)->getType()->dump() -> prints i32 The immediate issue is that you're setting v_jumpaddrp up as a pointer to a pointer to size_t - but then compare it to i32. I first was confused why the code tries to load the jump target dynamically. But then I saw that the interpreted code sets it dynamically - why? That's completely unnecessary overhead afaics? There's just two possible jump targets, no? - why is EvalJsonPathVar() in execExprInterp.c, when it's only ever called from within jsonpath_exec.c? - s/JsobbValue/JsonbValue/ Greetings, Andres Freund
pgsql-hackers by date: