Re: Questionable coding in nth_value

On Sat, May 6, 2023 at 4:44 PM Tatsuo Ishii <ishii(at)sraoss(dot)co(dot)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