Feedback-ID: ia2694551:Fastmail
Date: Sat, 29 Jun 2024 20:24:27 +0200
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Amit Langote <amitlangote09@gmail.com>
Cc: jian he <jian.universality@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>, 
	Peter Eisentraut <peter@eisentraut.org>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: pgsql: Add more SQL/JSON constructor functions
Message-ID: <202406291824.reofujy7xdj3@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CA+HiwqHEn0ZNK9koksrLTpJiwK8Y2pB4R-0tYjQmLy489_4XCQ@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/202406291824.reofujy7xdj3%40alvherre.pgsql>
Precedence: bulk

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