Questionable coding in nth_value

Started by Tatsuo Ishiiover 2 years ago3 messages

ishii@sraoss.co.jp

over 2 years ago

1 attachment(s)

Currently Window function nth_value is coded as following:

nth = DatumGetInt32(WinGetFuncArgCurrent(winobj, 1, &isnull));
if (isnull)
PG_RETURN_NULL();
const_offset = get_fn_expr_arg_stable(fcinfo->flinfo, 1);

if (nth <= 0)
ereport(ERROR,
:
:

Is there any reason why argument 'nth' is not checked earlier?
IMO, it is more natural "if (nth <= 0)..." is placed right after "nth = DatumGetInt32...".

Attached is the patch which does this.

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

Attachments:

fix_nth_value.patchtext/x-patch; charset=us-asciiDownload

diff --git a/src/backend/utils/adt/windowfuncs.c b/src/backend/utils/adt/windowfuncs.c
index b87a624fb2..f4ff060930 100644
--- a/src/backend/utils/adt/windowfuncs.c
+++ b/src/backend/utils/adt/windowfuncs.c
@@ -696,15 +696,16 @@ window_nth_value(PG_FUNCTION_ARGS)
 	int32		nth;
 
 	nth = DatumGetInt32(WinGetFuncArgCurrent(winobj, 1, &isnull));
-	if (isnull)
-		PG_RETURN_NULL();
-	const_offset = get_fn_expr_arg_stable(fcinfo->flinfo, 1);
-
 	if (nth <= 0)
 		ereport(ERROR,
 				(errcode(ERRCODE_INVALID_ARGUMENT_FOR_NTH_VALUE),
 				 errmsg("argument of nth_value must be greater than zero")));
 
+	if (isnull)
+		PG_RETURN_NULL();
+
+	const_offset = get_fn_expr_arg_stable(fcinfo->flinfo, 1);
+
 	result = WinGetFuncArgInFrame(winobj, 0,
 								  nth - 1, WINDOW_SEEK_HEAD, const_offset,
 								  &isnull, NULL);

Richard Guo

guofenglinux@gmail.com

over 2 years ago

In reply to: Tatsuo Ishii (#1)

Re: Questionable coding in nth_value

On Sat, May 6, 2023 at 4:44 PM Tatsuo Ishii <ishii@sraoss.co.jp> wrote:

Currently Window function nth_value is coded as following:

nth = DatumGetInt32(WinGetFuncArgCurrent(winobj, 1, &isnull));
if (isnull)
PG_RETURN_NULL();
const_offset = get_fn_expr_arg_stable(fcinfo->flinfo, 1);

if (nth <= 0)
ereport(ERROR,
:
:

Is there any reason why argument 'nth' is not checked earlier?
IMO, it is more natural "if (nth <= 0)..." is placed right after "nth =
DatumGetInt32...".

Attached is the patch which does this.

Hmm, shouldn't we check if the argument of nth_value is null before we
check if it is greater than zero? So maybe we need to do this.

--- a/src/backend/utils/adt/windowfuncs.c
+++ b/src/backend/utils/adt/windowfuncs.c
@@ -698,13 +698,14 @@ window_nth_value(PG_FUNCTION_ARGS)
    nth = DatumGetInt32(WinGetFuncArgCurrent(winobj, 1, &isnull));
    if (isnull)
        PG_RETURN_NULL();
-   const_offset = get_fn_expr_arg_stable(fcinfo->flinfo, 1);

if (nth <= 0)
ereport(ERROR,
(errcode(ERRCODE_INVALID_ARGUMENT_FOR_NTH_VALUE),
errmsg("argument of nth_value must be greater than
zero")));

+   const_offset = get_fn_expr_arg_stable(fcinfo->flinfo, 1);
+
    result = WinGetFuncArgInFrame(winobj, 0,
                                  nth - 1, WINDOW_SEEK_HEAD, const_offset,
                                  &isnull, NULL);

Thanks
Richard

Tatsuo Ishii

ishii@sraoss.co.jp

over 2 years ago

In reply to: Richard Guo (#2)

Re: Questionable coding in nth_value

On Sat, May 6, 2023 at 4:44 PM Tatsuo Ishii <ishii@sraoss.co.jp> wrote:

Currently Window function nth_value is coded as following:

nth = DatumGetInt32(WinGetFuncArgCurrent(winobj, 1, &isnull));
if (isnull)
PG_RETURN_NULL();
const_offset = get_fn_expr_arg_stable(fcinfo->flinfo, 1);

if (nth <= 0)
ereport(ERROR,
:
:

Is there any reason why argument 'nth' is not checked earlier?
IMO, it is more natural "if (nth <= 0)..." is placed right after "nth =
DatumGetInt32...".

Attached is the patch which does this.

Hmm, shouldn't we check if the argument of nth_value is null before we
check if it is greater than zero? So maybe we need to do this.

That makes sense. I thought since this function is marked as strict,
it would not be called if argument is NULL, but I was wrong.

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