public inbox for [email protected]  

help / color / mirror / Atom feed

From: Tatsuo Ishii <[email protected]>
To: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Subject: Re: Add RESPECT/IGNORE NULLS and FROM FIRST/LAST options
Date: Mon, 13 Oct 2025 14:39:41 +0900 (JST)
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
	<[email protected]>
	<[email protected]>

>>>> Also the error is certainly user-facing,
>>>> so using elog() was quite inappropriate.  It should be ereport with an
>>>> errcode of (probably) ERRCODE_FEATURE_NOT_SUPPORTED.  Rolling your
>>>> own implementation of get_func_name() wasn't great either.
>>> 
>>> I overlooked the elog() call and "own implementation of
>>> get_func_name()". Will fix.
>> 
>> Attached is a trivial patch to fix that. I am going to push it if
>> there's no objection.
>> 
>> Best regards,
>> --
>> Tatsuo Ishii
>> SRA OSS K.K.
>> English: http://www.sraoss.co.jp/index_en/
>> Japanese:http://www.sraoss.co.jp
>> <v1-0001-Use-ereport-rather-than-elog-in-WinCheckAndInitia.patch>
> 
> 
> I just take a quick look at the patch, a tiny comment is:
> 
> ```
> +		char	   *funcname = get_func_name(fcinfo->flinfo->fn_oid);
> ```
> 
> This can be a “const char *”.

Thanks for the review. In addition to the point, I added an assertion
which is called by all other window function API. Also added check to
the return value of get_func_name() because it could return NULL. V2
patch attached.

Best regards,
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp



Attachments:

  [application/octet-stream] v2-0001-Use-ereport-rather-than-elog-in-WinCheckAndInitia.patch (2.2K, ../[email protected]/2-v2-0001-Use-ereport-rather-than-elog-in-WinCheckAndInitia.patch)
  download | inline diff:
From 976de4af0951a49e9fa55fad03f0582a4e77e7f5 Mon Sep 17 00:00:00 2001
From: Tatsuo Ishii <[email protected]>
Date: Mon, 13 Oct 2025 14:32:01 +0900
Subject: [PATCH v2] Use ereport rather than elog in
 WinCheckAndInitializeNullTreatment.

Previously WinCheckAndInitializeNullTreatment() used elog() to emit an
error message. ereport() should be used instead because it's a
user-facing error. Also use existing get_func_name() to get a
function's name, rather than own implementation.

In addition to them, add an assertion to validate winobj parameter,
just like other window function API.

Reported-by: Tom Lane <[email protected]>
Author: Tatsuo Ishii <[email protected]>
Reviewed-by: Chao Li <[email protected]>
Discussion: https://postgr.es/m/2952409.1760023154%40sss.pgh.pa.us
---
 src/backend/executor/nodeWindowAgg.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/src/backend/executor/nodeWindowAgg.c b/src/backend/executor/nodeWindowAgg.c
index e6a53f95391..47e00be7b49 100644
--- a/src/backend/executor/nodeWindowAgg.c
+++ b/src/backend/executor/nodeWindowAgg.c
@@ -3538,24 +3538,20 @@ WinCheckAndInitializeNullTreatment(WindowObject winobj,
 								   bool allowNullTreatment,
 								   FunctionCallInfo fcinfo)
 {
+	Assert(WindowObjectIsValid(winobj));
 	if (winobj->ignore_nulls != NO_NULLTREATMENT && !allowNullTreatment)
 	{
-		HeapTuple	proctup;
-		Form_pg_proc procform;
-		Oid			funcid;
+		const char *funcname = get_func_name(fcinfo->flinfo->fn_oid);
 
-		funcid = fcinfo->flinfo->fn_oid;
-		proctup = SearchSysCache1(PROCOID,
-								  ObjectIdGetDatum(funcid));
-		if (!HeapTupleIsValid(proctup))
-			elog(ERROR, "cache lookup failed for function %u", funcid);
-		procform = (Form_pg_proc) GETSTRUCT(proctup);
-		elog(ERROR, "function %s does not allow RESPECT/IGNORE NULLS",
-			 NameStr(procform->proname));
+		if (!funcname)
+			elog(ERROR, "could not get function name");
+		ereport(ERROR,
+				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+				 errmsg("function %s does not allow RESPECT/IGNORE NULLS",
+						funcname)));
 	}
 	else if (winobj->ignore_nulls == PARSER_IGNORE_NULLS)
 		winobj->ignore_nulls = IGNORE_NULLS;
-
 }
 
 /*
-- 
2.43.0

view thread (91+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: Add RESPECT/IGNORE NULLS and FROM FIRST/LAST options
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox