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