On Wed, Apr 05, 2023 at 05:39:35PM -0700, Andres Freund wrote:
> Seems like a complicated enough facility to benefit from a test or two? Peter
> Eisentraut added support for the extended query protocol to psql, so it
> shouldn't be too hard...
PQsendQueryGuts() does not split yet the bind/describe
On Wed, Apr 05, 2023 at 10:16:19PM +, Imseih (AWS), Sami wrote:
> Here is v6. That was my mistake not to zero out the es_total_processed.
> I had it in the first version.
The update of es_total_processed in standard_ExecutorRun() felt a bit
lonely, so I have added an extra comment, ran an inde
Hi,
On 2023-04-06 07:09:37 +0900, Michael Paquier wrote:
> I'll look at that again today, potentially apply the fix on HEAD.
Seems like a complicated enough facility to benefit from a test or two? Peter
Eisentraut added support for the extended query protocol to psql, so it
shouldn't be too hard.
> Makes sense to me. I'll look at that again today, potentially apply
> the fix on HEAD.
Here is v6. That was my mistake not to zero out the es_total_processed.
I had it in the first version.
--
Regards,
Sami Imseih
Amazon Web Services (AWS)
v6-0001-Fix-row-tracking-in-pg_stat_statements.pat
On Wed, Apr 05, 2023 at 05:39:13PM -0400, Tom Lane wrote:
> v5 seems OK to me except I think CreateExecutorState() should explicitly
> zero the new es_total_processed field, alongside zeroing es_processed.
> (I realize that the makeNode would have done it already, but our
> coding conventions gener
Michael Paquier writes:
> On Wed, Apr 05, 2023 at 04:07:21AM +, Imseih (AWS), Sami wrote:
>> Attached is v5 addressing the comments.
> Thanks, this should be enough to persist the number of tuples tracked
> across multiple ExecutorRun() calls. This looks pretty good to me.
v5 seems OK to me
On Wed, Apr 05, 2023 at 04:07:21AM +, Imseih (AWS), Sami wrote:
>> - es_processed: number of tuples processed during one ExecutorRun()
>> call.
>> - es_total_processed: total number of tuples aggregated across all
>> ExecutorRun() calls.
>
> I thought hard about this point and for some reason
> 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.
I think we will need to clearly define what "calls" is. Perhaps as mentioned
above, we may need separat
On Tue, Apr 04, 2023 at 09:48:17PM +, 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_process
> I was looking back at this thread, and the suggestion to use one field
> in EState sounds fine to me. Sami, would you like to send a new
> version of the patch (simplified version based on v1)?
Here is v4.
The "calls" tracking is removed from Estate. Unlike v1 however,
I added a check for the o
On Tue, Apr 04, 2023 at 03:29:07AM +, Imseih (AWS), Sami wrote:
>> 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.
>
> Sure, the original proposed fix, but with tracking the es_tota
> Maybe, but is there any field demand for that?
I don't think there is.
> 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.
Sure, the original proposed fix, but with tracking the es_tota
"Imseih (AWS), Sami" writes:
> 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?
Maybe, but is there any field demand for that?
IMV, the existing behavior is that we count one
> 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.
Correct, and there are also dr
"Imseih (AWS), Sami" writes:
>> 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.
> * 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
"Imseih (AWS), Sami" 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
> 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 call
On Thu, Mar 23, 2023 at 01:54:05PM +, Imseih (AWS), Sami wrote:
> 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://
> 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 u
On Thu, 23 Mar 2023 09:33:16 +0100
"Drouvot, Bertrand" wrote:
> 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
> >> al
> How does JDBC test that? Does it have a dependency on
> pg_stat_statements?
No, at the start of the thread, a sample jdbc script was attached.
But I agree, we need to add test coverage. See below.
>> But, I'm tempted to say that adding new tests could be addressed
>> separately though (as this
On Thu, Mar 23, 2023 at 09:33:16AM +0100, Drouvot, Bertrand wrote:
> Thanks! LGTM and also do confirm that, with the patch, the JDBC test
> 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 y
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)
Th
> 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 me
Hi,
On 3/21/23 2:16 PM, Imseih (AWS), Sami wrote:
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 t
> 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 mo
On Mon, Mar 20, 2023 at 09:41:12PM +, Imseih (AWS), Sami wrote:
> 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.
>
> If this is more palatable, I can prepare the patch.
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
@@
It appears you must "make clean; make install" to correctly compile after
applying the patch.
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
Thank you all for pointing out that it needs make clean first.
Hi,
On 3/2/23 8:27 AM, Michael Paquier wrote:
On Wed, Jan 25, 2023 at 11:22:04PM +, Imseih (AWS), Sami wrote:
Doing some work with extended query protocol, I encountered the same
issue that was discussed in [1]. It appears when a client is using
extended query protocol and sends an Execute
On Sat, Mar 11, 2023 at 11:55:22PM +, Imseih (AWS), Sami wrote:
> It appears you must "make clean; make install" to correctly compile after
> applying the patch.
In a git repository, I've learnt to rely on this simple formula, even
if it means extra cycles when running ./configure:
git clean -
> 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)
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 fa
> Well, it is one of these areas where it seems to me we have never been
> able to put a definition on what should be the correct behavior when
> it comes to pg_stat_statements.
What needs to be defined here is how pgss should account for # of rows
processed when A) a select goes through extended
On Wed, Jan 25, 2023 at 11:22:04PM +, Imseih (AWS), Sami wrote:
> Doing some work with extended query protocol, I encountered the same
> issue that was discussed in [1]. It appears when a client is using
> extended query protocol and sends an Execute message to a portal with
> max_rows, and a p
Doing some work with extended query protocol, I encountered the same
issue that was discussed in [1]. It appears when a client is using
extended query protocol and sends an Execute message to a portal with
max_rows, and a portal is executed multiple times,
pg_stat_statements does not correctly trac
37 matches
Mail list logo
