Re: CommandStatus from insert returning when using a portal.

On Fri, Jul 14, 2023 at 12:51 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> "David G. Johnston" <david(dot)g(dot)johnston(at)gmail(dot)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.

https://www.postgresql.org/docs/current/sql-insert.html

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.

https://github.com/postgres/postgres/commit/2f9661311b83dc481fc19f6e3bda015392010a40#diff-f66f9adc3dfc98f2ede2e96691843b75128689a8cb9b79ae68d53dc749c3b22bL781

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 */

> 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.