Questionable coding in nth_value
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t47827psql -h localhost -U postgresBuilt from patchset v1 (message #1), October 25, 2026 at 04:02 AM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t47827_1 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t47827_1 && git checkout t47827_1Patchset v1 (message #1) is on t47827_1
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
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
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