MIME-Version: 1.0
References: <1cee81f3-312f-30e3-0282-c4df7ee72f93@xs4all.nl>
 <202403041503.5momp6knupgo@alvherre.pgsql> <CA+HiwqGtdVCg-KboUUU=svwukwXGT4EOFOk3eCZLz6h-d6YLNw@mail.gmail.com>
 <CAPF61jAOJM6FqMsPGYHXbKxtX8DQ2a7kuOZ3AzR8oGXHy3fjFA@mail.gmail.com>
 <562e5ad9-1357-4752-ad28-7e56740f74f8@enterprisedb.com> <CAPF61jBm+_QCwDg32_K+BgNKf09isU9g3o8CLW2kUDLZ5=28Hw@mail.gmail.com>
 <f1084e8b-17f2-4e1f-9d75-fbb48eb75280@enterprisedb.com> <CA+HiwqF5CHyRBO8WwOcQsKufFWZ1yjDaQQ7DJxsQNbm2nab3pg@mail.gmail.com>
 <CA+HiwqHjQcQaUx63Z65KnBdDg=xWiKKH84zNL=T+Gg4bKPS8dw@mail.gmail.com> <CACJufxE4NDVY_Xeg=ip96MBw2u-U=qe6ar2qo2D2PQm-vDeDaQ@mail.gmail.com>
In-Reply-To: <CACJufxE4NDVY_Xeg=ip96MBw2u-U=qe6ar2qo2D2PQm-vDeDaQ@mail.gmail.com>
From: jian he <jian.universality@gmail.com>
Date: Wed, 20 Mar 2024 14:51:48 +0800
Message-ID: <CACJufxFkKV8-tMa88ByaabYDQan9+cFCcf1Z1jH7=bQuMAHN4A@mail.gmail.com>
Subject: Re: remaining sql/json patches
To: Amit Langote <amitlangote09@gmail.com>
Cc: Tomas Vondra <tomas.vondra@enterprisedb.com>, 
	Himanshu Upadhyaya <upadhyaya.himanshu@gmail.com>, Alvaro Herrera <alvherre@alvh.no-ip.org>, 
	Erik Rijkers <er@xs4all.nl>, Andres Freund <andres@anarazel.de>, Andrew Dunstan <andrew@dunslane.net>, 
	PostgreSQL-development <pgsql-hackers@postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Archived-At: <https://www.postgresql.org/message-id/CACJufxFkKV8-tMa88ByaabYDQan9%2BcFCcf1Z1jH7%3DbQuMAHN4A%40mail.gmail.com>
Precedence: bulk

minor issues I found while looking through it.
other than these issues, looks good!

/*
 * Convert the a given JsonbValue to its C string representation
 *
 * Returns the string as a Datum setting *resnull if the JsonbValue is a
 * a jbvNull.
 */
static char *
ExecGetJsonValueItemString(JsonbValue *item, bool *resnull)
{
}
I think the comments are not right?

/*
 * Checks if the coercion evaluation led to an error.  If an error did occur,
 * this sets post_eval->error to trigger the ON ERROR handling steps.
 */
void
ExecEvalJsonCoercionFinish(ExprState *state, ExprEvalStep *op)
{
}
these comments on ExecEvalJsonCoercionFinish also need to be updated?


+ /*
+ * Coerce the result value by calling the input function coercion.
+ * *op->resvalue must point to C string in this case.
+ */
+ if (!*op->resnull && jsexpr->use_io_coercion)
+ {
+ FunctionCallInfo fcinfo;
+
+ fcinfo = jsestate->input_fcinfo;
+ Assert(fcinfo != NULL);
+ Assert(val_string != NULL);
+ fcinfo->args[0].value = PointerGetDatum(val_string);
+ fcinfo->args[0].isnull = *op->resnull;
+ /* second and third arguments are already set up */
+
+ fcinfo->isnull = false;
+ *op->resvalue = FunctionCallInvoke(fcinfo);
+ if (SOFT_ERROR_OCCURRED(&jsestate->escontext))
+ error = true;
+
+ jump_eval_coercion = -1;
+ }

+ /* second and third arguments are already set up */
change to
/* second and third arguments are already set up in ExecInitJsonExpr */
would be great.


commit message
<<<<
All of these functions only operate on jsonb values. The workaround
for now is to cast the argument to jsonb.
<<<<
should be removed?


+ case T_JsonFuncExpr:
+ {
+ JsonFuncExpr *jfe = (JsonFuncExpr *) node;
+
+ if (WALK(jfe->context_item))
+ return true;
+ if (WALK(jfe->pathspec))
+ return true;
+ if (WALK(jfe->passing))
+ return true;
+ if (jfe->output && WALK(jfe->output))
+ return true;
+ if (jfe->on_empty)
+ return true;
+ if (jfe->on_error)
+ return true;
+ }

+ if (jfe->output && WALK(jfe->output))
+ return true;
can be simplified:

+ if (WALK(jfe->output))
+ return true;