Subject: Re: pgsql: Add more SQL/JSON constructor functions

From: Alvaro Herrera <[email-masked]>
Date: Sat, 29 Jun 2024 20:24:27 +0200

> diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c
> index 233b7b1cc9..df766cdec1 100644
> --- a/src/backend/parser/parse_expr.c
> +++ b/src/backend/parser/parse_expr.c
> @@ -3583,6 +3583,7 @@ coerceJsonFuncExpr(ParseState *pstate, Node *expr,
>  	Node	   *res;
>  	int			location;
>  	Oid			exprtype = exprType(expr);
> +	int32		baseTypmod = returning->typmod;
>  
>  	/* if output type is not specified or equals to function type, return */
>  	if (!OidIsValid(returning->typid) || returning->typid == exprtype)
> @@ -3611,10 +3612,19 @@ coerceJsonFuncExpr(ParseState *pstate, Node *expr,
>  		return (Node *) fexpr;
>  	}
>  
> +	/*
> +	 * For domains, consider the base type's typmod to decide whether to setup
> +	 * an implicit or explicit cast.
> +	 */
> +	if (get_typtype(returning->typid) == TYPTYPE_DOMAIN)
> +		(void) getBaseTypeAndTypmod(returning->typid, &baseTypmod);

I didn't review this patch in detail, but I just noticed this tiny bit
and wanted to say that I don't like this coding style where you
initialize a variable to a certain value, and much later you override it
with a completely different value.  It seems much clearer to leave it
uninitialized at first, and have both places that determine the value
together,

    if (get_typtype(returning->typid) == TYPTYPE_DOMAIN)
        (void) getBaseTypeAndTypmod(returning->typid, &baseTypmod);
    else
        baseTypmod = returning->typmod;

Not only because in the domain case the initializer value is a downright
lie, but also because of considerations such as if you later add code
that uses the variable in between those two places, you'd be introducing
a bug in the domain case because it hasn't been set.  With the coding I
propose, the compiler immediately tells you that the initialization is
missing.

TBH I'm not super clear on why we decide on explicit or implicit cast
based on presence of a typmod.  Why isn't it better to always use an
implicit one?

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Small aircraft do not crash frequently ... usually only once!"
                                  (ponder, http://thedailywtf.com/)

---

From: jian he <[email-masked]>
Date: Mon, 1 Jul 2024 15:10:00 +0800

> based on presence of a typmod.  Why isn't it better to always use an

I am using an example to explain it.
SELECT JSON_SERIALIZE(JSON('{ "a" : 1 } '));
we cannot directly use implicit cast from json to text in
{coerceJsonFuncExpr, coerce_to_target_type}
because function calls:
coerceJsonFuncExpr->coerce_to_target_type->can_coerce_type
->find_coercion_pathway
will look up pg_cast entries.
but we don't have text & json implicit cast entries, we will fail at:

````
if (!res && report_error)
ereport(ERROR,
errcode(ERRCODE_CANNOT_COERCE),
errmsg("cannot cast type %s to %s",
   format_type_be(exprtype),
   format_type_be(returning->typid)),
parser_coercion_errposition(pstate, location, expr));
````

Most of the cast uses explicit cast, which is what we previously did,
then in this thread, we found out for the returning type typmod(
(varchar, or varchar's domain)
We need to first cast the expression to text then text to varchar via
implicit cast.
To trap the error:
for example: SELECT JSON_SERIALIZE('{ "a" : 1 } ' RETURNING varchar(2);
also see the comment:
https://git.postgresql.org/cgit/postgresql.git/commit/?id=c2d93c3802b205d135d1ae1d7ac167d74e08a274
+ /*
+ * Convert the source expression to text, because coerceJsonFuncExpr()
+ * will create an implicit cast to the RETURNING types with typmod and
+ * there are no implicit casts from json(b) to such types. For domains,
+ * the base type's typmod will be considered, so do so here too.
+ */
In general, I think implicit cast here is an exception.

overall I come up with following logic:
-----------------
int32 baseTypmod = -1;
if (returning->typmod < 0)
(void) getBaseTypeAndTypmod(returning->typid, &baseTypmod);
else
baseTypmod = returning->typmod;

res = coerce_to_target_type(pstate, expr, exprtype,
returning->typid, baseTypmod,
baseTypmod > 0 ? COERCION_IMPLICIT :
COERCION_EXPLICIT,
baseTypmod > 0 ? COERCE_IMPLICIT_CAST :
COERCE_EXPLICIT_CAST,
location);
-----------------
By the same way we are dealing with varchar,
I came up with a verbose patch for transformJsonBehavior,
which can cope with all the corner cases of bit and varbit data type.
I also attached a test sql file (scratch169.sql) for it.
some examples:
--fail
SELECT JSON_VALUE(jsonb '"111a"', '$'  RETURNING bit(3) default '1111'
on error);
--ok
SELECT JSON_VALUE(jsonb '"111a"', '$'  RETURNING bit(3) default '111' on error);
--ok
SELECT JSON_VALUE(jsonb '"111a"', '$'  RETURNING bit(3) default 32 on error);

makeJsonConstructorExpr we called (void)
getBaseTypeAndTypmod(returning->typid, &baseTypmod);
later in coerceJsonFuncExpr
we may also call (void) getBaseTypeAndTypmod(returning->typid, &baseTypmod);
maybe we can do some refactoring.