Re: [BUG] pg_stat_statements and extended query protocol
От | Michael Paquier |
---|---|
Тема | Re: [BUG] pg_stat_statements and extended query protocol |
Дата | |
Msg-id | ZCzhk1f51fejSsMB@paquier.xyz обсуждение исходный текст |
Ответ на | Re: [BUG] pg_stat_statements and extended query protocol ("Imseih (AWS), Sami" <simseih@amazon.com>) |
Ответы |
Re: [BUG] pg_stat_statements and extended query protocol
("Imseih (AWS), Sami" <simseih@amazon.com>)
|
Список | pgsql-hackers |
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
Вложения
В списке pgsql-hackers по дате отправления:
Предыдущее
От: Amit LangoteДата:
Сообщение: Re: on placeholder entries in view rule action query's range table
Следующее
От: Michael PaquierДата:
Сообщение: Re: POC: Lock updated tuples in tuple_update() and tuple_delete()