Re: pgsql: Add more SQL/JSON constructor functions

> 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/)