public inbox for [email protected]
help / color / mirror / Atom feedFrom: Tatsuo Ishii <[email protected]>
To: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Subject: Re: Add RESPECT/IGNORE NULLS and FROM FIRST/LAST options
Date: Thu, 16 Oct 2025 19:17:06 +0900 (JST)
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<[email protected]>
<[email protected]>
Thanks for the report.
> Coverity thinks that this code has still some incorrect bits, and I
> think that it is right to think so even on today's HEAD at
> 02c171f63fca.
>
> In WinGetFuncArgInPartition()@nodeWindowAgg.c, we have the following
> loop (keeping only the relevant parts:
> do
> {
> [...]
> else /* need to check NULL or not */
> {
> /* get tuple and evaluate in partition */
> datum = gettuple_eval_partition(winobj, argno,
> abs_pos, isnull, &myisout);
> if (myisout) /* out of partition? */
> break;
> if (!*isnull)
> notnull_offset++;
> /* record the row status */
> put_notnull_info(winobj, abs_pos, *isnull);
> }
> } while (notnull_offset < notnull_relpos);
>
> /* get tuple and evaluate in partition */
> datum = gettuple_eval_partition(winobj, argno,
> abs_pos, isnull, &myisout);
>
> And Coverity is telling that there is no point in setting a datum in
> this else condition to just override its value when we exit the while
> loop. To me, it's a sigh that this code's logic could be simplified.
To fix the issue, I think we can change:
> datum = gettuple_eval_partition(winobj, argno,
> abs_pos, isnull, &myisout);
to:
(void) gettuple_eval_partition(winobj, argno,
abs_pos, isnull, &myisout);
This explicitely stats that we ignore the return value from
gettuple_eval_partition. I hope coverity understands this.
> In passing, gettuple_eval_partition() is under-documented for me. Its
> name refers to the fact that it gets a tuple and evaluates a
> partition. Its top comment tells the same thing as the name of the
> function, so it's a bit hard to say why it is useful with the code
> written this way, and how others many benefit when attempting to reuse
> it, or if it even makes sense to reuse it for other purposes.
What about changing the comment this way?
/* gettuple_eval_partition
* get tuple in a patition and evaluate the window function's argument
* expression on it.
*/
Attached is the patch for above.
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
Attachments:
[application/octet-stream] v1-0001-Fix-coverity-complaint-about-WinGetFuncArgInParti.patch (2.4K, ../[email protected]/2-v1-0001-Fix-coverity-complaint-about-WinGetFuncArgInParti.patch)
download | inline diff:
From c18dcbccff7bd7c95295beedabb2116cbb3b3be2 Mon Sep 17 00:00:00 2001
From: Tatsuo Ishii <[email protected]>
Date: Thu, 16 Oct 2025 19:00:56 +0900
Subject: [PATCH v1] Fix coverity complaint about WinGetFuncArgInPartition.
Coverity complains that the return value from gettuple_eval_partition
in a do..while loop in WinGetFuncArgInPartition is overwritten when
exiting the while loop. This commit tries to fix the issue by changing
gettuple_eval_partition call to:
(void) gettuple_eval_partition()
explicitly stating that we discard the return value.
Also enhance some comments for easier code reading.
Discussion: https://postgr.es/m/aPCOabSE4VfJLaky%40paquier.xyz
---
src/backend/executor/nodeWindowAgg.c | 19 +++++++++++++------
1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/src/backend/executor/nodeWindowAgg.c b/src/backend/executor/nodeWindowAgg.c
index 47e00be7b49..aa145d4e1a9 100644
--- a/src/backend/executor/nodeWindowAgg.c
+++ b/src/backend/executor/nodeWindowAgg.c
@@ -3270,8 +3270,9 @@ window_gettupleslot(WindowObject winobj, int64 pos, TupleTableSlot *slot)
return true;
}
-/*
- * get tuple and evaluate in partition
+/* gettuple_eval_partition
+ * get tuple in a patition and evaluate the window function's argument
+ * expression on it.
*/
static Datum
gettuple_eval_partition(WindowObject winobj, int argno,
@@ -3790,9 +3791,15 @@ WinGetFuncArgInPartition(WindowObject winobj, int argno,
continue; /* keep on moving forward or backward */
else /* need to check NULL or not */
{
- /* get tuple and evaluate in partition */
- datum = gettuple_eval_partition(winobj, argno,
- abs_pos, isnull, &myisout);
+ /*
+ * NOT NULL info does not exist yet. Get tuple and evaluate func
+ * arg in partition. We ignore the return value from
+ * gettuple_eval_partition because we are just interested in
+ * whether we are inside or outside of partition, NULL or NOT
+ * NULL.
+ */
+ (void) gettuple_eval_partition(winobj, argno,
+ abs_pos, isnull, &myisout);
if (myisout) /* out of partition? */
break;
if (!*isnull)
@@ -3802,7 +3809,7 @@ WinGetFuncArgInPartition(WindowObject winobj, int argno,
}
} while (notnull_offset < notnull_relpos);
- /* get tuple and evaluate in partition */
+ /* get tuple and evaluate func arg in partition */
datum = gettuple_eval_partition(winobj, argno,
abs_pos, isnull, &myisout);
if (!myisout && set_mark)
--
2.43.0
view thread (91+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Add RESPECT/IGNORE NULLS and FROM FIRST/LAST options
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox