Обсуждение: Questionable coding in nth_value
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 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);
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
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
> 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