> * Yeah, it'd be nice to have an in-core test, but it's folly to insist
> on one that works via libpq and psql. That requires a whole new set
> of features that you're apparently designing on-the-fly with no other
> use cases in mind. I don't think that will accomplish much except to
> ensure that this bug fix doesn't make it into v16.
I agree, Solving the lack of in-core testing for this area belong in a
different discussion.
> * I don't understand why it was thought good to add two new counters
> to struct Instrumentation. In EXPLAIN ANALYZE cases those will be
> wasted space *per plan node*, not per Query.
Indeed, looking at ExplainNode, Instrumentation struct is allocated
per node and the new fields will be wasted space. Thanks for highlighting
this.
> * It also seems quite bizarre to add counters to a core data structure
> and then leave it to pg_stat_statements to maintain them.
That is fair point
> I'm inclined to think that adding the counters to struct EState is
> fine. That's 304 bytes already on 64-bit platforms, another 8 or 16
> won't matter.
With the point you raise about Insrumentation per node, Estate
is the better place for the new counters.
> Also, I'm doubtful that counting calls this way is a great idea,
> which would mean you only need one new counter field not two. The
> fact that you're having trouble defining what it means certainly
> suggests that the implementation is out front of the design.
ISTM you are not in agreement that a call count should be incremented
after every executorRun, but should only be incremented after
the portal is closed, at executorEnd. Is that correct?
FWIW, The rationale for incrementing calls in executorRun is that calls refers
to the number of times a client executes a portal, whether partially or to completion.
Clients can also fetch rows from portals at various rates, so to determine the
"rows per call" accurately from pg_stat_statements, we should track a calls as
the number of times executorRun was called on a portal.
> In short, what I think I'd suggest is adding an es_total_processed
> field to EState and having standard_ExecutorRun do "es_total_processed
> += es_processed" near the end, then just change pg_stat_statements to
> use es_total_processed not es_processed.
The original proposal in 0001-correct-pg_stat_statements-tracking-of-portals.patch,
was to add a "calls" and "es_total_processed" field to EState.
Regards,
Sami Imseih
Amazon Web Services (AWS)
