Re: [BUG] pg_stat_statements and extended query protocol

"Imseih (AWS), Sami" <simseih(at)amazon(dot)com> writes:
>> So... The idea here is to set a custom fetch size so as the number of
>> calls can be deterministic in the tests, still more than 1 for the
>> tests we'd have. And your point is that libpq enforces always 0 when
>> sending the EXECUTE message causing it to always return all the rows
>> for any caller of PQsendQueryGuts().

> That is correct.

Hi, I took a quick look through this thread, and I have a couple of
thoughts:

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

* It also seems quite bizarre to add counters to a core data structure
and then leave it to pg_stat_statements to maintain them. (BTW, I didn't
much care for putting that maintenance into pgss_ExecutorRun without
updating its header comment.)

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.

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.

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.

regards, tom lane