public inbox for [email protected]
help / color / mirror / Atom feedFrom: Paul A Jungwirth <[email protected]>
To: Tom Lane <[email protected]>
Cc: Peter Eisentraut <[email protected]>
Cc: Chao Li <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: SQL:2011 Application Time Update & Delete
Date: Sun, 19 Apr 2026 11:51:23 -0700
Message-ID: <CA+renyVdyBcLzh2sFWUJ44A+N+7qL=f7rAyDOqFsTHTDb5q=SA@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<[email protected]>
<CA+renyUazgR-hB_6RY60n23L0y-n_h9G1AappZmPENO0k5pL1g@mail.gmail.com>
<[email protected]>
<CA+renyVXg5pV84wQnGQuK8-=qoKw3BiBgQzesxM_LkcxxWmYjA@mail.gmail.com>
<[email protected]>
<CA+renyWKOj5=rMmQmJcbybu-Vdomxdp=eJ93kp76AgmQKYdfiQ@mail.gmail.com>
<[email protected]>
<CA+renyUhuXB2nTVCMREXew9E4DZOnFxQNjME5bcw91+k72Bosg@mail.gmail.com>
<CA+renyWUCSyTMn3s03kviEN-oaVrJP-QkDQCLNfaY=MHV5QEiQ@mail.gmail.com>
<CA+renyV4tWU2d=n9_v=XNPHbZfNqqLokzd-Xt78M-zLd+46ubA@mail.gmail.com>
<[email protected]>
<CA+renyUSgqXpjj+vV7w+wirPB49VQFrmPjVT_s04JmZSOPNNsQ@mail.gmail.com>
<[email protected]>
<CA+renyX-eV+2hFUaZg3BSREqLE7dh+LoWm7ZqhFAiGsirjjtRQ@mail.gmail.com>
<[email protected]>
<[email protected]>
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]
Attachments:
[text/x-patch] v2-0001-Make-FOR-PORTION-OF-obey-SRF-protocol.patch (2.4K, 2-v2-0001-Make-FOR-PORTION-OF-obey-SRF-protocol.patch)
download | inline diff:
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
view thread (54+ 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]
Subject: Re: SQL:2011 Application Time Update & Delete
In-Reply-To: <CA+renyVdyBcLzh2sFWUJ44A+N+7qL=f7rAyDOqFsTHTDb5q=SA@mail.gmail.com>
* 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