Subject: Re: [BUG] pg_stat_statements and extended query protocol

From: David Zhang <[email-masked]>
Date: Fri, 10 Mar 2023 12:58:23 -0800

> Yes, I agree that proper test coverage is needed here. Will think
> about how to accomplish this.

Tried to apply this patch to current master branch and the build was ok, 
however it crashed during initdb with a message like below.

"performing post-bootstrap initialization ... Segmentation fault (core 
dumped)"

If I remove this patch and recompile again, then "initdb -D $PGDATA" works.

Thanks,

David

---

From: "Imseih (AWS), Sami" <[email-masked]>
Date: Sat, 11 Mar 2023 23:55:22 +0000

> If I remove this patch and recompile again, then "initdb -D $PGDATA" works.

It appears you must "make clean; make install" to correctly compile after
applying the patch.

Regards,

Sami Imseih
Amazon Web Services (AWS)

---

From: Michael Paquier <[email-masked]>
Date: Sun, 12 Mar 2023 11:57:36 +0900

> It appears you must "make clean; make install" to correctly compile after

In a git repository, I've learnt to rely on this simple formula, even
if it means extra cycles when running ./configure:
git clean -d -x -f
--
Michael

---

From: David Zhang <[email-masked]>
Date: Mon, 13 Mar 2023 14:30:55 -0700

> if it means extra cycles when running ./configure:
Thank you all for pointing out that it needs make clean first. After 
make clean followed by recompile with the patch then both make check 
from regression test and pg_stat_statements extension report all test 
passed. So the current existing test cases can't really detect any 
change from this patch, then it would be better to add some test cases 
to cover this.

Best regards,

David

---

From: "Imseih (AWS), Sami" <[email-masked]>
Date: Mon, 20 Mar 2023 21:41:12 +0000

Sorry about the delay in response about this.

I was thinking about this and it seems to me we can avoid
adding new fields to Estate. I think a better place to track
rows and calls is in the Instrumentation struct.

--- a/src/include/executor/instrument.h
+++ b/src/include/executor/instrument.h
@@ -88,6 +88,8 @@ typedef struct Instrumentation
        double          nfiltered2;             /* # of tuples removed by "other" quals */
        BufferUsage bufusage;           /* total buffer usage */
        WalUsage        walusage;               /* total WAL usage */
+       int64       calls;
+       int64       rows_processed;
 } Instrumentation; 

If this is more palatable, I can prepare the patch.

Thanks for your feedback.

Regards.

Sami Imseih

---

From: Michael Paquier <[email-masked]>
Date: Tue, 21 Mar 2023 10:04:46 +0900

> If this is more palatable, I can prepare the patch.

This indeed feels a bit more natural seen from here, after looking at
the code paths using an Instrumentation in the executor and explain,
for example.  At least, this stresses me much less than adding 16
bytes to EState for something restricted to the extended protocol when
it comes to monitoring capabilities.
--
Michael

---

From: "Imseih (AWS), Sami" <[email-masked]>
Date: Tue, 21 Mar 2023 13:16:29 +0000

> it comes to monitoring capabilities.

Attached is the patch that uses Instrumentation. 

I did not add any new tests, and we do not have anyway now
of setting a row count when going through the Execute message.
I think this may be need to be addressed separately since there
Seems to be. Gap in extended query protocol testing.

For this fix however, The JDBC test does show correct results.

Regards,

Sami Imseih

---

From: "Drouvot, Bertrand" <[email-masked]>
Date: Wed, 22 Mar 2023 12:04:23 +0100

Hi,

> Attached is the patch that uses Instrumentation.

Thanks, I think this new approach makes sense.

-                                          const BufferUsage *bufusage,
+                                          int64 calls, const BufferUsage *bufusage,

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)

@@ -88,6 +88,8 @@ typedef struct Instrumentation
         double          nfiltered2;             /* # of tuples removed by "other" quals */
         BufferUsage bufusage;           /* total buffer usage */
         WalUsage        walusage;               /* total WAL usage */
+       int64       calls;                      /* # of total calls to ExecutorRun */
+       int64       rows_processed;     /* # of total rows processed in ExecutorRun */

I'm not sure it's worth mentioning that the new counters are "currently" used with the ExecutorRun.

What about just "total calls" and "total rows processed" (or "total rows", see below)?

Also, I wonder if "rows" (and not rows_processed) would not be a better naming.

Those last comments regarding the Instrumentation are done because ISTM that at the end their usage
could vary depending of the use case of the Instrumentation.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

---

From: "Imseih (AWS), Sami" <[email-masked]>
Date: Wed, 22 Mar 2023 21:35:23 +0000

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

Regards,

---

From: "Drouvot, Bertrand" <[email-masked]>
Date: Thu, 23 Mar 2023 09:33:16 +0100

Hi,

> 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,

---

From: Michael Paquier <[email-masked]>
Date: Thu, 23 Mar 2023 18:21:21 +0900

> does show the correct results.

How does JDBC test that?  Does it have a dependency on
pg_stat_statements?
> 
> That said, not having a test (for the reasons you explained
> up-thread) associated with the patch worry me a bit.

Same impression here.

> But, I'm tempted to say that adding new tests could be addressed
> separately though (as this patch looks pretty straightforward).

Even small patches can have gotchas.  I think that this should have
tests in-core rather than just depend on JDBC and hope for the best.
Even if \bind does not allow that, we could use an approach similar to
libpq_pipeline, for example, depending on pg_stat_statements for the
validation with a test module in src/test/modules/?
--
Michael

---

From: "Imseih (AWS), Sami" <[email-masked]>
Date: Thu, 23 Mar 2023 13:54:05 +0000

> How does JDBC test that? Does it have a dependency on

No, at the start of the thread, a sample jdbc script was attached.
But I agree, we need to add test coverage. See below.

> validation with a test module in src/test/modules/?

Yes, that is possible but we will need to add a libpq API
that allows the caller to pass in a "fetch size".
PQsendQueryParams does not take in a "fetch size", 
so it returns all rows, through PQsendQueryParams

https://github.com/postgres/postgres/blob/master/src/interfaces/libpq/fe-exec.c#L1882

Adding such an API that takes in a "fetch size" will be beneficial 
not just for this test, but I can see it enabling another psql meta command,
similar to \bind but that takes in a "fetch size".

Regards

---

From: Yugo NAGATA <[email-masked]>
Date: Fri, 24 Mar 2023 12:21:44 +0900

On Thu, 23 Mar 2023 09:33:16 +0100

> > v3 addresses the comments.

I wonder that this patch changes the meaning of "calls" in the pg_stat_statement
view a bit; previously it was "Number of times the statement was executed" as
described in the documentation, but currently this means  "Number of times the
portal was executed". I'm worried that this makes users confused. For example,
a user may think the average numbers of rows returned by a statement is given by
rows/calls, but it is not always correct because some statements could be executed
with multiple portal runs. 

Although it might not big issue to users, I think it is better to add an explanation
to the doc for clarification.

Regards,
Yugo Nagata

-- 
Yugo NAGATA <[email-masked]>

---

From: "Imseih (AWS), Sami" <[email-masked]>
Date: Fri, 24 Mar 2023 13:32:43 +0000

> with multiple portal runs.

I don't think it changes the meaning of "calls" in pg_stat_statements, since every
time the app fetches X amount of rows from a portal, it's still done in a separate
execution, and thus a separate call.

I agree, the meaning of "calls" should be clarified in docs.

Regards,

Sami Imseih

---

From: Michael Paquier <[email-masked]>
Date: Wed, 29 Mar 2023 15:59:20 +0900

> similar to \bind but that takes in a "fetch size".

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().

The extended protocol allows that, so you would like a libpq API to
have more control of what we send with EXECUTE:
https://www.postgresql.org/docs/current/protocol-overview.html#PROTOCOL-QUERY-CONCEPTS

The extended query protocol would require multiple 'E' messages, but
we would not need multiple describe or bind messages, meaning that
this cannot just be an extra flavor PQsendQueryParams().  Am I gettig
that right?  The correct API design seems tricky, to say the least.
Perhaps requiring this much extra work in libpq for the purpose of
having some tests in this thread is not a brilliant idea..  Or perhaps
we could just do it and have something a-la-JDBC with two routines?
That would be one libpq routine for describe/bind and one for execute
where the limit can be given by the caller in the latter case, similar
to sendDescribeStatement() and sendExecute() in
QueryExecutorImpl.java.
--
Michael

---

From: "Imseih (AWS), Sami" <[email-masked]>
Date: Fri, 31 Mar 2023 18:06:46 +0000

> for any caller of PQsendQueryGuts().

That is correct.

> that right?  

Correct, there will need to be separate APIs for Parse/Bind, Execute
and Close of a Portal.

> The correct API design seems tricky, to say the least.

I am not too clear on your point here. ISTM you are suggesting adding
new libpq APis similar to JDBC, which is what I am also suggesting.

Did I understand correctly?

---

From: Tom Lane <[email-masked]>
Date: Mon, 03 Apr 2023 12:43:41 -0400

> 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

---

From: "Imseih (AWS), Sami" <[email-masked]>
Date: Tue, 4 Apr 2023 02:19:46 +0000

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

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

> and then leave it to pg_stat_statements to maintain them. 

That is fair point

> fine. That's 304 bytes already on 64-bit platforms, another 8 or 16

With the point you raise about Insrumentation per node, Estate 
is the better place for the new counters.

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

> 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

---

From: Tom Lane <[email-masked]>
Date: Mon, 03 Apr 2023 22:47:11 -0400

> the portal is closed, at executorEnd. Is that correct?

Right.  That makes the "call count" equal to the number of times the
query is invoked.

> to the number of times a client executes a portal, whether partially or to completion.

Why should that be the definition?  Partial execution of a portal
might be something that is happening at the driver level, behind the
user's back.  You can't make rational calculations of, say, plan
time versus execution time if that's how "calls" is measured.

			regards, tom lane

---

From: "Imseih (AWS), Sami" <[email-masked]>
Date: Tue, 4 Apr 2023 03:01:05 +0000

> time versus execution time if that's how "calls" is measured.

Correct, and there are also drivers that implement fetch size using
cursor statements, i.e. DECLARE CURSOR, FETCH CURSOR,
and each FETCH gets counted as 1 call.

I wonder if the right answer here is to track fetches as 
a separate counter in pg_stat_statements, in which fetch
refers to the number of times a portal is executed?

Regards,

Sami Imseih

---

From: Tom Lane <[email-masked]>
Date: Mon, 03 Apr 2023 23:13:12 -0400

> refers to the number of times a portal is executed?

Maybe, but is there any field demand for that?

IMV, the existing behavior is that we count one "call" per overall
query execution (that is, per ExecutorEnd invocation).  The argument
that that's a bug and we should change it seems unsupportable to me,
and even the argument that we should also count ExecutorRun calls
seems quite lacking in evidence.  We clearly do need to fix the
reported rowcount for cases where ExecutorRun is invoked more than
once per ExecutorEnd call; but I think that's sufficient.

			regards, tom lane