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