Hi,
On 3/22/23 10:35 PM, Imseih (AWS), Sami wrote:
What about using an uint64 for calls? That seems more appropriate to me (even if
queryDesc->totaltime->calls will be passed (which is int64), but that's already
also the case for the "rows" argument and queryDesc->totaltime->rows_processed)
That's fair
I'm not sure it's worth mentioning that the new counters are "currently" used
with the ExecutorRun.
Sure, I suppose these fields could be used outside of ExecutorRun. Good point.
Also, I wonder if "rows" (and not rows_processed) would not be a better naming.
Agree.
I went with rows_processed initially, since it was accumulating es_processed,
but as the previous point, this instrumentation could be used outside of
ExecutorRun.
v3 addresses the comments.
Thanks! LGTM and also do confirm that, with the patch, the JDBC test does show
the correct results.
That said, not having a test (for the reasons you explained up-thread)
associated with the patch worry me a bit.
But, I'm tempted to say that adding new tests could be addressed separately
though (as this patch looks pretty straightforward).
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
