On Sun, Apr 19, 2026 at 11:10 AM Tom Lane <[email protected]> wrote:
>
> Peter Eisentraut <[email protected]> writes:
> > I have committed the patches 0001 through 0003.
>
> Coverity is complaining that rsi.isDone may be used uninitialized in
> ExecForPortionOfLeftovers. It's correct: that function is not obeying
> the function call protocol, and it's only accidental that it's not
> failing. In ValuePerCall mode the caller is supposed to initialize
> isDone (and isnull too) before each call. The canonical reference
> for this is execSRF.c, and it does that. So I think we need something
> like the attached.
Thanks for the patch! Your changes look good to me.
> I notice that execSRF.c also runs pgstat_init_function_usage and
> pgstat_end_function_usage around each call. That's not too important
> right now, but I wonder whether we should add it while we're looking
> at this. It would perhaps be important once we support user-defined
> withoutPortionProcs.
I agree we should do that. Here is a patch with that added to your changes.
I was curious why execSRF.c uses `rsinfo.isDone != ExprMultipleResult`
for the finalize parameter, because I don't think a SRF should ever
return ExprSingleResult, right? So I guess it is just to be cautious.
Makes sense. I followed that approach.
Yours,
--
Paul ~{:-)
[email protected]
From ebb58c1af3eb280abec988d258c349cc9377ae67 Mon Sep 17 00:00:00 2001
From: "Paul A. Jungwirth" <[email protected]>
Date: Sun, 19 Apr 2026 11:30:17 -0700
Subject: [PATCH v2] Make FOR PORTION OF obey SRF protocol
This fixes a Coverity error about rsi.isDone not being initialized. The built-in
{multi,}range_minus_multi functions don't return without setting it, but a
user-supplied function might not be as accommodating.
We also add statistics tracking around the function call.
---
src/backend/executor/nodeModifyTable.c | 20 ++++++++++++++++++--
1 file changed, 18 insertions(+), 2 deletions(-)
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index ef2a6bc6e9d..353a05cadff 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -65,6 +65,7 @@
#include "miscadmin.h"
#include "nodes/nodeFuncs.h"
#include "optimizer/optimizer.h"
+#include "pgstat.h"
#include "rewrite/rewriteHandler.h"
#include "rewrite/rewriteManip.h"
#include "storage/lmgr.h"
@@ -1419,6 +1420,7 @@ ExecForPortionOfLeftovers(ModifyTableContext *context,
CmdType oldOperation;
TransitionCaptureState *oldTcs;
FmgrInfo flinfo;
+ PgStat_FunctionCallUsage fcusage;
ReturnSetInfo rsi;
bool didInit = false;
bool shouldFree = false;
@@ -1514,6 +1516,7 @@ ExecForPortionOfLeftovers(ModifyTableContext *context,
rsi.expectedDesc = NULL;
rsi.allowedModes = (int) (SFRM_ValuePerCall);
rsi.returnMode = SFRM_ValuePerCall;
+ /* isDone is filled below */
rsi.setResult = NULL;
rsi.setDesc = NULL;
@@ -1537,14 +1540,27 @@ ExecForPortionOfLeftovers(ModifyTableContext *context,
*/
while (true)
{
- Datum leftover = FunctionCallInvoke(fcinfo);
+ Datum leftover;
+
+ pgstat_init_function_usage(fcinfo, &fcusage);
+
+ /* Call the function one time */
+ fcinfo->isnull = false;
+ rsi.isDone = ExprSingleResult;
+ leftover = FunctionCallInvoke(fcinfo);
+
+ pgstat_end_function_usage(&fcusage,
+ rsi.isDone != ExprMultipleResult);
+
+ if (rsi.returnMode != SFRM_ValuePerCall)
+ elog(ERROR, "without_portion function violated function call protocol");
/* Are we done? */
if (rsi.isDone == ExprEndResult)
break;
if (fcinfo->isnull)
- elog(ERROR, "Got a null from without_portion function");
+ elog(ERROR, "got a null from without_portion function");
/*
* Does the new Datum violate domain checks? Row-level CHECK
--
2.47.3
