Re: Questionable coding in nth_value

Поиск
Список
Период
Сортировка
От Richard Guo
Тема Re: Questionable coding in nth_value
Дата
Msg-id CAMbWs48=9moEFp4u-czzrB9GeT+aTEB4otTbJudYHwb0MuM4MA@mail.gmail.com
обсуждение исходный текст
Ответ на Questionable coding in nth_value  (Tatsuo Ishii <ishii@sraoss.co.jp>)
Ответы Re: Questionable coding in nth_value
Список pgsql-hackers
Дерево обсуждения

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

В списке pgsql-hackers по дате отправления: