Re: [BUG] pg_stat_statements and extended query protocol

From: "Imseih (AWS), Sami" <simseih(at)amazon(dot)com>
To: Michael Paquier <michael(at)paquier(dot)xyz>
Cc: "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: [BUG] pg_stat_statements and extended query protocol
Date: 2023-03-06 14:19:46
Message-ID: 9B9BB0F2-68A3-44E0-946E-3D9051F83369@amazon.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

> 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 query (EP) protocol, and
B) it requires multiple executes to complete a portal.

The patch being suggested will treat every 'E' ( execute message ) to the same
portal as a new call ( pgss will increment the calls ) and the number of rows
processed will be accumulated for every 'E' message.

Currently, only the rows fetched in the last 'E' call to the portal is tracked by
pgss. This is incorrect.

> Could it be possible to add some
> regression tests using the recently-added \bind command and see how
> this affects things?

\bind alone will not be enough as we also need a way to fetch from
a portal in batches. The code that needs to be exercised
as part of the test is exec_execute_message with max_rows != 0.

\bind will call exec_execute_message with max_rows = 0 to fetch
all the rows.

> I would suggest splitting these into their own
> SQL file, following an effort I have been doing recently for the
> regression tests of pg_stat_statements. It would be good to know the
> effects of this change for pg_stat_statements.track = (top|all), as
> well.

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

> - uint64 es_processed; /* # of tuples processed */
> + uint64 es_processed; /* # of tuples processed at the top level only */
> + uint64 es_calls; /* # of calls */
> + uint64 es_total_processed; /* total # of tuples processed */

> So the root of the logic is here. Anything that makes the executor
> structures larger freaks me out, FWIW, and that's quite an addition.

I am not sure how to get around the changes to Estate and fixing this
Issue.

We could potentially only need the es_total_processed field and
continue to track calls in pgss.

es_total_processed in EState however is still needed.

Regards,

--

Sami Imseih
Amazon Web Services (AWS)

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Ashutosh Bapat 2023-03-06 14:30:03 Re: Inaccurate comment for pg_get_partkeydef
Previous Message Ashutosh Bapat 2023-03-06 14:14:42 Re: wrong results due to qual pushdown