Re: [BUG] pg_stat_statements and extended query protocol

On Tue, Apr 04, 2023 at 09:48:17PM +0000, Imseih (AWS), Sami wrote:
> The "calls" tracking is removed from Estate. Unlike v1 however,
> I added a check for the operation type. Inside ExecutorRun,
> es_total_processed is incremented when the operation is
> a SELECT. This check is done for es_processed as well inside
> executorRun -> ExecutePlan.

I see. This seems in line with ExecutePlan() where es_processed is
incremented only for SELECT queries.

> For non select operations, es_total_processed is set to
> es_processed in executorfinish. This is because the modify
> plan nodes set es_processed outside of execMain.c

My java is very rusty but (after some time reminding myself how to do
a CLASSPATH) I can see the row counts being right for things like
SELECT, INSERT RETURNING, WITH queries for both of them and
autocommit, and the proposed fix influences SELECT only depending on
the fetch size. Doing nothing for calls now is fine by me, though I
agree that this could be improved at some point, as seeing only 1
rather than N for each fetch depending on the size is a bit confusing.

* There is no return value, but output tuples (if any) are sent to
* the destination receiver specified in the QueryDesc; and the number
* of tuples processed at the top level can be found in
* estate->es_processed.

Doesn't this comment at the top of ExecutorRun() need an update? It
seems to me that this comment should mention both es_total_processed
and es_processed, telling that es_processed is the number of rows
processed in a single call, while es_total_processed is the sum of all
tuples processed in the ExecutorRun() calls.

@@ -441,6 +451,13 @@ standard_ExecutorFinish(QueryDesc *queryDesc)
if (queryDesc->totaltime)
InstrStopNode(queryDesc->totaltime, 0);

+ /*
+ * For non-SELECT operations, es_total_processed will always be
+ * equal to es_processed.
+ */
+ if (operation != CMD_SELECT)
+ queryDesc->estate->es_total_processed = queryDesc->estate->es_processed;

There is no need for this part in ExecutorFinish(), actually, as long
as we always increment es_total_processed at the end ExecutorRun() for
all the operation types? If the portal does not have a store, we
would do one ExecutorRun() call per execute fetch. If the portal has
a store, we'd do only one ExecutorRun(). Both cases call once
ExecutorFinish(), but the finish would happen during the first
execute when filling a portal's tuple store, and during the last
execute fetch if the portal has no store. This is Tom's point in [1],
from what I can see. That seems simpler to me, as well.

- uint64 es_processed; /* # of tuples processed */
+ uint64 es_processed; /* # of tuples processed for a single
+ * execution of a portal */
+ uint64 es_total_processed; /* # of tuples processed for all
+ * executions of a portal */

Hmm. This does not reflect completely the reality for non-SELECT
statements, no? For SELECT statements, that's correct, because
es_processed is reset in standard_ExecutorFinish() each time the
backend does an execute fetch thanks to PortalRunSelect(). For
non-SELECT statements, the extended query protocol uses the tuples
stored in the portal after one execution, meaning that we run through
the executor once with both es_processed and es_total_processed set to
their final numbers from the start, before any fetches. I would
suggest something like that to document both fields:
- es_processed: number of tuples processed during one ExecutorRun()
call.
- es_total_processed: total number of tuples aggregated across all
ExecutorRun() calls.

At the end, I'm OK with the proposal after a closer look, but I think
that we should do a much better job at describing es_processed and
es_total_processed in execnodes.h, particularly in the case of a
portal holding a store where es_processed may not reflect the number
of rows for a single portal execution, and it seems to me that the
proposal of incrementing es_total_processed at the end of
ExecutorRun() for all commands is simpler, based on what I have
tested.

[1]: https://www.postgresql.org/message-id/1311773.1680577992@sss.pgh.pa.us
--
Michael