Re: PG 15 (and to a smaller degree 14) regression due to ExprEvalStep size

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 instruction are 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