Subject: Re: remaining sql/json patches

From: Alvaro Herrera <[email-masked]>
Date: Tue, 19 Sep 2023 12:18:49 +0200

0001: I wonder why you used Node for the ErrorSaveContext pointer
instead of the specific struct you want.  I propose the attached, for
some extra type-safety.  Or did you have a reason to do it that way?

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"How amazing is that? I call it a night and come back to find that a bug has
been identified and patched while I sleep."                (Robert Davidson)
               http://archives.postgresql.org/pgsql-sql/2006-03/msg00378.php
>From 4cd888cd7abbfb156bad26bc94658be3e286bf1f Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <[email-masked]>
Date: Tue, 19 Sep 2023 12:18:03 +0200
Subject: [PATCH] 0001 fixup: use struct ErrorSaveContext, not Node

---
 src/backend/jit/llvm/llvmjit.c       | 4 ++--
 src/backend/jit/llvm/llvmjit_expr.c  | 2 +-
 src/backend/jit/llvm/llvmjit_types.c | 2 +-
 src/include/executor/execExpr.h      | 3 ++-
 src/include/jit/llvmjit.h            | 2 +-
 src/include/nodes/execnodes.h        | 3 ++-
 6 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/src/backend/jit/llvm/llvmjit.c b/src/backend/jit/llvm/llvmjit.c
index ef6c8361b4..431d4511c5 100644
--- a/src/backend/jit/llvm/llvmjit.c
+++ b/src/backend/jit/llvm/llvmjit.c
@@ -85,7 +85,7 @@ LLVMTypeRef StructExprState;
 LLVMTypeRef StructAggState;
 LLVMTypeRef StructAggStatePerGroupData;
 LLVMTypeRef StructAggStatePerTransData;
-LLVMTypeRef StructNode;
+LLVMTypeRef StructErrorSaveContext;
 
 LLVMValueRef AttributeTemplate;
 
@@ -1035,7 +1035,7 @@ llvm_create_types(void)
 	StructAggState = llvm_pg_var_type("StructAggState");
 	StructAggStatePerGroupData = llvm_pg_var_type("StructAggStatePerGroupData");
 	StructAggStatePerTransData = llvm_pg_var_type("StructAggStatePerTransData");
-	StructNode = llvm_pg_var_type("StructNode");
+	StructErrorSaveContext = llvm_pg_var_type("StructErrorSaveContext");
 
 	AttributeTemplate = LLVMGetNamedFunction(llvm_types_module, "AttributeTemplate");
 }
diff --git a/src/backend/jit/llvm/llvmjit_expr.c b/src/backend/jit/llvm/llvmjit_expr.c
index 20c79dda9f..59e20ebcbd 100644
--- a/src/backend/jit/llvm/llvmjit_expr.c
+++ b/src/backend/jit/llvm/llvmjit_expr.c
@@ -1361,7 +1361,7 @@ llvm_compile_expr(ExprState *state)
 						v_params[2] = l_int32_const(ioparam);
 						v_params[3] = l_int32_const(-1);
 						v_params[4] = l_ptr_const(op->d.iocoerce.escontext,
-												  l_ptr(StructNode));
+												  l_ptr(StructErrorSaveContext));
 						/*
 						 * InputFunctionCallSafe() will writes directly into
 						 * *op->resvalue.
diff --git a/src/backend/jit/llvm/llvmjit_types.c b/src/backend/jit/llvm/llvmjit_types.c
index 36f526b374..e1e9625038 100644
--- a/src/backend/jit/llvm/llvmjit_types.c
+++ b/src/backend/jit/llvm/llvmjit_types.c
@@ -67,7 +67,7 @@ TupleTableSlot StructTupleTableSlot;
 HeapTupleTableSlot StructHeapTupleTableSlot;
 MinimalTupleTableSlot StructMinimalTupleTableSlot;
 TupleDescData StructTupleDescData;
-Node StructNode;
+ErrorSaveContext StructErrorSaveContext;

 /*
diff --git a/src/include/executor/execExpr.h b/src/include/executor/execExpr.h
index 25d3c02dba..8dd92f8fc0 100644
--- a/src/include/executor/execExpr.h
+++ b/src/include/executor/execExpr.h
@@ -16,6 +16,7 @@
 
 #include "executor/nodeAgg.h"
 #include "nodes/execnodes.h"
+#include "nodes/miscnodes.h"
 
 /* forward references to avoid circularity */
 struct ExprEvalStep;
@@ -417,7 +418,7 @@ typedef struct ExprEvalStep
 			/* lookup and call info for result type's input function */
 			FmgrInfo   *finfo_in;
 			Oid			typioparam;
-			Node	   *escontext;
+			ErrorSaveContext *escontext;
 		}			iocoerce;
 
 		/* for EEOP_SQLVALUEFUNCTION */
diff --git a/src/include/jit/llvmjit.h b/src/include/jit/llvmjit.h
index e8cdc96b2b..c22ba3787f 100644
--- a/src/include/jit/llvmjit.h
+++ b/src/include/jit/llvmjit.h
@@ -79,7 +79,7 @@ extern PGDLLIMPORT LLVMTypeRef StructExprState;
 extern PGDLLIMPORT LLVMTypeRef StructAggState;
 extern PGDLLIMPORT LLVMTypeRef StructAggStatePerTransData;
 extern PGDLLIMPORT LLVMTypeRef StructAggStatePerGroupData;
-extern PGDLLIMPORT LLVMTypeRef StructNode;
+extern PGDLLIMPORT LLVMTypeRef StructErrorSaveContext;
 
 extern PGDLLIMPORT LLVMValueRef AttributeTemplate;
 
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 57e4ca1c63..cc4ed9279c 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -34,6 +34,7 @@
 #include "fmgr.h"
 #include "lib/ilist.h"
 #include "lib/pairingheap.h"
+#include "nodes/miscnodes.h"
 #include "nodes/params.h"
 #include "nodes/plannodes.h"
 #include "nodes/tidbitmap.h"
@@ -134,7 +135,7 @@ typedef struct ExprState
 	 * Used by the callers of ExecInitExprRec() to pass the ErrorSaveContext
 	 * that they want the expression's code to use.
 	 */
-	Node	   *escontext;
+	ErrorSaveContext *escontext;
 } ExprState;

-- 
2.39.2

---

From: Amit Langote <[email-masked]>
Date: Tue, 19 Sep 2023 20:56:43 +0900

> some extra type-safety.  Or did you have a reason to do it that way?

No reason other than that most other headers use Node.  I agree that
making an exception for this patch might be better, so I've
incorporated your patch into 0001.

I've also updated the query functions patch (0003) to address the
crashing bug reported by Erik.  Essentially, I made the coercion step
of JSON_QUERY to always use json_populate_type() when WITH WRAPPER is
used.  You might get funny errors with ERROR OR ERROR for many types
when used in RETURNING, but at least there should no longer be any
crashes.

-- 
Thanks, Amit Langote
EDB: http://www.enterprisedb.com

---

From: Erik Rijkers <[email-masked]>
Date: Tue, 19 Sep 2023 14:34:04 +0200

> when used in RETURNING, but at least there should no longer be any

Indeed, with v16 those crashes are gone.

Some lesser evil: gcc 13.2.0 gave some warnings, slightly different in 
assert vs non-assert build.

--- assert build:

-- [2023.09.19 14:06:35 json_table2/0] make core: make --quiet -j 4
In file included from ../../../src/include/postgres.h:45,
                  from parse_expr.c:16:
In function ‘transformJsonFuncExpr’,
     inlined from ‘transformExprRecurse’ at parse_expr.c:374:13:
parse_expr.c:4355:22: warning: ‘jsexpr’ may be used uninitialized 
[-Wmaybe-uninitialized]
  4355 |         Assert(jsexpr->formatted_expr);
../../../src/include/c.h:864:23: note: in definition of macro ‘Assert’
   864 |                 if (!(condition)) \
       |                       ^~~~~~~~~
parse_expr.c: In function ‘transformExprRecurse’:
parse_expr.c:4212:21: note: ‘jsexpr’ was declared here
  4212 |         JsonExpr   *jsexpr;
       |                     ^~~~~~

--- non-assert build:

-- [2023.09.19 14:11:03 json_table2/1] make core: make --quiet -j 4
In function ‘transformJsonFuncExpr’,
     inlined from ‘transformExprRecurse’ at parse_expr.c:374:13:
parse_expr.c:4356:28: warning: ‘jsexpr’ may be used uninitialized 
[-Wmaybe-uninitialized]
  4356 |         if (exprType(jsexpr->formatted_expr) != JSONBOID)
       |                      ~~~~~~^~~~~~~~~~~~~~~~
parse_expr.c: In function ‘transformExprRecurse’:
parse_expr.c:4212:21: note: ‘jsexpr’ was declared here
  4212 |         JsonExpr   *jsexpr;
       |                     ^~~~~~

Thank you,

Erik