Re: remaining sql/json patches

From: jian he <jian(dot)universality(at)gmail(dot)com>
To: Amit Langote <amitlangote09(at)gmail(dot)com>
Cc: Tomas Vondra <tomas(dot)vondra(at)enterprisedb(dot)com>, Himanshu Upadhyaya <upadhyaya(dot)himanshu(at)gmail(dot)com>, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>, Erik Rijkers <er(at)xs4all(dot)nl>, Andres Freund <andres(at)anarazel(dot)de>, Andrew Dunstan <andrew(at)dunslane(dot)net>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: remaining sql/json patches
Date: 2024-03-20 06:51:48
Message-ID: CACJufxFkKV8-tMa88ByaabYDQan9+cFCcf1Z1jH7=bQuMAHN4A@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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;

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message John Naylor 2024-03-20 06:57:54 Re: add AVX2 support to simd.h
Previous Message John Naylor 2024-03-20 06:48:10 Re: [PoC] Improve dead tuple storage for lazy vacuum