Re: CommandStatus from insert returning when using a portal. - Mailing list pgsql-hackers
| From | David G. Johnston |
|---|---|
| Subject | Re: CommandStatus from insert returning when using a portal. |
| Date | |
| Msg-id | CAKFQuwYhgyp3ioYrfFpjJzKP214-RTwQQv48WA-Q_JUWPwS-5g@mail.gmail.com Whole thread Raw |
| In response to | Re: CommandStatus from insert returning when using a portal. (Tom Lane <tgl@sss.pgh.pa.us>) |
| List | pgsql-hackers |
On Fri, Jul 14, 2023 at 12:51 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
"David G. Johnston" <david.g.johnston@gmail.com> writes:
> I agree that the documented contract of the insert command tag says it
> reports the size of the entire tuple store maintained by the server during
> the transaction instead of just the most recent count on subsequent fetches.
Where do you see that documented, exactly? I looked in the protocol
chapter and didn't find anything definite either way.
On successful completion, an INSERT command returns a command tag of the form
INSERT oid count
The count is the number of rows inserted or updated.
It doesn't, nor should, have any qualifications about not applying to the returning case and definitely shouldn't change based upon use of FETCH on the unnamed portal.
I'm quite prepared to believe there are bugs here, since this whole
set of behaviors is unreachable via libpq: you can't get it to send
an Execute with a count other than zero (ie, fetch all), nor is it
prepared to deal with the PortalSuspended messages it'd get if it did.
I think that the behavior is arising from this bit in PortalRun:
if (qc && portal->qc.commandTag != CMDTAG_UNKNOWN)
{
CopyQueryCompletion(qc, &portal->qc);
------>>> qc->nprocessed = nprocessed;
}
I came to the same conclusion. The original introduction of that line replaced string munging "SELECT " + nprocessed; so this code never even considered INSERT as being in scope and indeed wanted to return the per-run value in a fetch-list context.
I think I see why simple removal of that line is sufficient as the copied-from &portal->qc already has the result of executing the underlying insert query. That said, the paranoid approach would seem to be to keep the assignment but only use it when we aren't dealing with the returning case.
diff --git a/src/backend/tcop/pquery.c b/src/backend/tcop/pquery.c
index 5565f200c3..5e75141f0b 100644
--- a/src/backend/tcop/pquery.c
+++ b/src/backend/tcop/pquery.c
@@ -775,7 +775,8 @@ PortalRun(Portal portal, long count, bool isTopLevel, bool run_once,
if (qc && portal->qc.commandTag != CMDTAG_UNKNOWN)
{
CopyQueryCompletion(qc, &portal->qc);
- qc->nprocessed = nprocessed;
+ if (portal->strategy != PORTAL_ONE_RETURNING)
+ qc->nprocessed = nprocessed;
}
/* Mark portal not active */
index 5565f200c3..5e75141f0b 100644
--- a/src/backend/tcop/pquery.c
+++ b/src/backend/tcop/pquery.c
@@ -775,7 +775,8 @@ PortalRun(Portal portal, long count, bool isTopLevel, bool run_once,
if (qc && portal->qc.commandTag != CMDTAG_UNKNOWN)
{
CopyQueryCompletion(qc, &portal->qc);
- qc->nprocessed = nprocessed;
+ if (portal->strategy != PORTAL_ONE_RETURNING)
+ qc->nprocessed = nprocessed;
}
/* Mark portal not active */
It seems plausible to me that we should just remove that marked line.
Not sure about the compatibility implications, though.
I believe it is a bug worth fixing, save driver writers processing time getting a count when the command tag is supposed to be providing it to them using compute time already spent anyways.
David J.
pgsql-hackers by date: