Thread
Commits
-
Show sizes of FETCH queries as constants in pg_stat_statements
- bee23ea4ddc4 19 (unreleased) landed
-
Improve explicit cursor handling in pg_stat_statements
Sami Imseih <samimseih@gmail.com> — 2025-04-30T17:27:31Z
Hi hackers, I recently looked into a workload that makes heavy use of explicit cursors, and I found that pg_stat_statements can become a bottleneck. The application in question declares hundreds of cursors, and for each one, performs many FETCH and MOVE operations with varying fetch sizes. As a result, pg_stat_statements ends up overwhelmed by the deallocation (and garbage collection) of DECLARE CURSOR, FETCH, and MOVE entries. Each of these is associated with a unique queryId, which leads to bloated entries with limited diagnostic value. Other issues: 1. FETCH/MOVE statements don't really help much in laying blame on a specific query. the DECLARE CURSOR statement could have been evicted in pg_stat_statements by that point or a similar cursor name is pointing to a different query. Also, FETCH doesn't aggregate for the same cursor — e.g., FETCH 10 c1 and FETCH 20 c1 show up as separate entries. 2. DECLARE CURSOR doesn't provide execution stats for the underlying SQL. Enabling pg_stat_statements.track = 'all' can expose the underlying SQL, but adds overhead.There’s also a bug: the toplevel column for the underlying query is still marked as "t", even though you must set track "all" to see it. Based on this, I propose the following improvements: 1. Better normalization of cursor utility commands: 2. Normalize the cursor name in CLOSE. 3. Normalize fetch/move sizes in FETCH and MOVE. Users can use the rows and calls columns to derive average fetch size. Ideally I would want to normalize the cursor name and generate the queryId f the FETCH statement based on the underlying query, but that is not possible to do that post parsing. (The above normalizations of these utility statements will reduce the bloat.) 4. Track the underlying query of a cursor by default, even when pg_stat_statements.track_utility = off. I’ve attached two patches that implement this. Here's a quick example: ``` begin; declare cs1 cursor for select from pg_class; declare cs2 cursor for select from pg_class; fetch 10 cs1; fetch 20 cs1; fetch 10 cs1; fetch 10 cs2; close cs1; close cs2; declare cs1 cursor for select from pg_attribute; SELECT calls, rows, query, toplevel FROM pg_stat_statements ORDER BY query COLLATE "C"; commit; ``` current state: ``` postgres=*# SELECT calls, rows, query, toplevel FROM pg_stat_statements ORDER BY query COLLATE "C"; calls | rows | query | toplevel -------+------+-----------------------------------------------------------------------------------------------------------+---------- 1 | 1 | SELECT name FROM pg_catalog.pg_available_extensions WHERE name LIKE $1 AND installed_version IS NULL+| t | | LIMIT $2 | 1 | 0 | begin | t 1 | 0 | close cs1 | t 1 | 0 | close cs2 | t 1 | 0 | create extension pg_stat_statements | t 1 | 0 | declare cs1 cursor for select from pg_attribute | t 1 | 0 | declare cs1 cursor for select from pg_class | t 1 | 0 | declare cs2 cursor for select from pg_class | t 2 | 20 | fetch 10 cs1 | t 1 | 10 | fetch 10 cs2 | t 1 | 20 | fetch 20 cs1 | t (11 rows) ``` with both patches applied: ``` postgres=*# SELECT calls, rows, query, toplevel FROM pg_stat_statements ORDER BY query COLLATE "C"; calls | rows | query | toplevel -------+------+------------------------------------------------+---------- 1 | 0 | begin | t 2 | 0 | close $1 | t 1 | 0 | declare $1 cursor for select from pg_attribute | t 2 | 0 | declare $1 cursor for select from pg_class | t 3 | 40 | fetch $1 cs1 | t 1 | 10 | fetch $1 cs2 | t 2 | 50 | select from pg_class | t (7 rows) postgres=*# commit; COMMIT ``` FWIW, I raised this ~3 years ago [0], but there was not much interest. I have seen this being a problem a few times since then that I think something should be done about. I also was not happy with the approach I took in [0]. Looking forward to feedback! Regards, -- Sami Imseih Amazon Web Services (AWS) [0] https://www.postgresql.org/message-id/flat/203CFCF7-176E-4AFC-A48E-B2CECFECD6AA%40amazon.com -
Re: Improve explicit cursor handling in pg_stat_statements
Sami Imseih <samimseih@gmail.com> — 2025-04-30T19:43:41Z
I forgot to add the proper tests to the Normalize cursor utility statements. Reattaching v2. I also want to add that the decision to not normalize the cursor name in the FETCH command is because it would not make sense to combine FETCH commands for various cursors into the same entry. Regards, -- Sami Imseih Amazon Web Services (AWS)
-
Re: Improve explicit cursor handling in pg_stat_statements
Michael Paquier <michael@paquier.xyz> — 2025-05-01T05:05:45Z
On Wed, Apr 30, 2025 at 02:43:41PM -0500, Sami Imseih wrote: > I also want to add that the decision to not normalize the cursor name in > the FETCH command is because it would not make sense to combine > FETCH commands for various cursors into the same entry. - calls | rows | query --------+------+------------------------------------------------------- - 2 | 0 | CLOSE cursor_stats_1 - 2 | 0 | DECLARE cursor_stats_1 CURSOR WITH HOLD FOR SELECT $1 + calls | rows | query +-------+------+---------------------------------------------------- + 2 | 0 | CLOSE $1 + 2 | 0 | DECLARE $1 CURSOR WITH HOLD FOR SELECT $2 Hmm. What are the workloads that you have seen as problematic? Do these involve cursor names generated randomly, where most of them are similar with a random factor for the name? Too much normalization here would limit the amount of verbosity that we have for this area, especially if we are dealing with query patterns that rely on few CLOSE naming patterns spread across a couple of key queries, because we would now know anymore about their distribution. - 1 | 5 | FETCH FORWARD 5 pgss_cursor - 1 | 7 | FETCH FORWARD ALL pgss_cursor - 1 | 1 | FETCH NEXT pgss_cursor + 1 | 0 | DECLARE $1 CURSOR FOR SELECT * FROM pgss_matv Saying that, applying normalization for the number of FETCH looks like a natural move. It seems to me that we should still make a difference with the ALL case, though? typedef struct ClosePortalStmt { NodeTag type; - char *portalname; /* name of the portal (cursor) */ + /* name of the portal (cursor) */ + char *portalname pg_node_attr(query_jumble_ignore); + ParseLoc location pg_node_attr(query_jumble_location); /* NULL means CLOSE ALL */ Could it matter to make a distinction with CLOSE ALL, compared to the case where the CLOSE statements are named? It would be possible to make a difference compared to the named case with an extra boolean field, for example. I would suggest to add some tests for CLOSE ALL anyway; we don't have any currently. -- Michael -
Re: Improve explicit cursor handling in pg_stat_statements
Sami Imseih <samimseih@gmail.com> — 2025-05-02T21:21:21Z
> Hmm. What are the workloads that you have seen as problematic? Do > these involve cursor names generated randomly, where most of them are > similar with a random factor for the name? postgres_fdw, as an example, in which cursor name get reused for different queries. Notice below "c1" and "c2" is reused for different queries, so now what underlying sql is FETCH, i.e. FETCH 100 FROM c1 referring to? v2-0001 does not help us with the FETCH problem because as I mentioned we don't have access to the underlying sql ( and parsing is even too early to do a portal lookup to find the underlying sql to base the queryId on). What v2-0001 will do is at least group the DECLARE CURSOR statements together for cursors referencing the same query and reduce the # of entries. ``` create foreign table t2(id int) server r1; create foreign table t1(id int) server r1; postgres=# select * from t2, t ; id | id ----+---- 1 | 1 (1 row) postgres=# select * from t, t2 ; id | id ----+---- 1 | 1 (1 row) ``` on the remote side ``` postgres=# select calls, query from pg_stat_statements where query like '% c%'; calls | query -------+----------------------------------------------------------------- 1 | DECLARE c2 CURSOR FOR + | SELECT id FROM public.t2 2 | DECLARE c1 CURSOR FOR + | SELECT id FROM public.t2 3 | CLOSE c2 3 | CLOSE c1 2 | DECLARE c2 CURSOR FOR + | SELECT id FROM public.t 3 | FETCH 100 FROM c1 3 | FETCH 100 FROM c2 1 | DECLARE c1 CURSOR FOR + | SELECT id FROM public.t 2 | select calls, query from pg_stat_statements where query like $1 (9 rows) ``` > Too much normalization > here would limit the amount of verbosity that we have for this area, > especially if we are dealing with query patterns that rely on few > CLOSE naming patterns spread across a couple of key queries, because > we would now know anymore about their distribution. The FETCH and CLOSE are already not clear to what underlying SQL they are referring to, and there is not much chance to actually improve that unless we track a cursor queryId in pg_stat_statements ( at that point we can show that FETCH or CLOSE refer to this specific cursor statement ). -- Sami -
Re: Improve explicit cursor handling in pg_stat_statements
Michael Paquier <michael@paquier.xyz> — 2025-05-08T06:35:54Z
On Fri, May 02, 2025 at 04:21:21PM -0500, Sami Imseih wrote: > postgres_fdw, as an example, in which cursor name get reused > for different queries. Notice below "c1" and "c2" is reused for different > queries, so now what underlying sql is FETCH, i.e. FETCH 100 FROM c1 referring > to? v2-0001 does not help us with the FETCH problem > because as I mentioned we don't have access to the underlying sql > ( and parsing is even too early to do a portal lookup to find the > underlying sql to > base the queryId on). What v2-0001 will do is at least group the DECLARE CURSOR > statements together for cursors referencing the same query and reduce the # > of entries. This case relies on postgres_fdw's GetCursorNumber() that assigns a unique number for a cursor, ensuring uniqueness per connection within a transaction, and the counter is reset at the end of the transactions. So good point for this case that this hurts. If that holds for the most common cases where this is seen as bloating pgss, that brings some solid ground, especially more for applications that use many cursor numbers in long-ish transactions states done under postgres_fdw. I'm still slightly worried about workloads where cursor names could be used to track some balancing of this kind of activity, for example if cursor names are fixed on a transaction-basis depending on the application involved, as that would mean less visibility in the information. Perhaps we would have more confidence by looking closer at drivers or some ORMs that make use of cursors, seeing how the end user would be impacted? That seems hard to measure, though.. > The FETCH and CLOSE are already not clear to what underlying SQL > they are referring to, and there is not much chance to actually > improve that unless > we track a cursor queryId in pg_stat_statements ( at that point we can show that > FETCH or CLOSE refer to this specific cursor statement ). I don't really have an issue for FETCH with the number as the name is still around, but I'm equally worrying about the loss of information for CLOSE that this new normalization would imply. Perhaps my worries don't have a reason to exist here and I'm just a naturally-pessimistic being. -- Michael
-
Re: Improve explicit cursor handling in pg_stat_statements
Sami Imseih <samimseih@gmail.com> — 2025-05-29T21:31:50Z
> > postgres_fdw, as an example, in which cursor name get reused > > for different queries. Notice below "c1" and "c2" is reused for different > > queries, so now what underlying sql is FETCH, i.e. FETCH 100 FROM c1 referring > > to? v2-0001 does not help us with the FETCH problem > > because as I mentioned we don't have access to the underlying sql > > ( and parsing is even too early to do a portal lookup to find the > > underlying sql to > > base the queryId on). What v2-0001 will do is at least group the DECLARE CURSOR > > statements together for cursors referencing the same query and reduce the # > > of entries. > > This case relies on postgres_fdw's GetCursorNumber() that assigns a > unique number for a cursor, ensuring uniqueness per connection within > a transaction, and the counter is reset at the end of the > transactions. So good point for this case that this hurts. If that > holds for the most common cases where this is seen as bloating pgss, > that brings some solid ground, especially more for applications that > use many cursor numbers in long-ish transactions states done under > postgres_fdw. Sorry for the delayed response. I’ve been thinking about this a bit, and I agree that it’s really hard to get a good sense of the use cases out there. postgres_fdw does have the issue of reusing cursor names, which renders the stats essentially meaningless, in my opinion—not to mention it contributes to excessive bloat. Looking at a driver like psycopg, explicit cursors are supported through the driver, but the user must define the name of the cursor, so it's much more controlled. I really wonder if the right answer is to have a pg_stat_statements.track_cursor_utility GUC that can toggle the reporting of utility statements related to explicit cursors, while still tracking the underlying statements. I would even suggest making 'on' the default to maintain the current behavior. I don’t like that we have to introduce a new GUC for this, but I can't think of a better alternative. Thoughts? -- Sami Imseih Amazon Web Services (AWS)
-
Re: Improve explicit cursor handling in pg_stat_statements
Sami Imseih <samimseih@gmail.com> — 2025-06-02T19:44:36Z
> > The FETCH and CLOSE are already not clear to what underlying SQL > > they are referring to, and there is not much chance to actually > > improve that unless > > we track a cursor queryId in pg_stat_statements ( at that point we can show that > > FETCH or CLOSE refer to this specific cursor statement ). > I don't really have an issue for FETCH with the number as the name is Since the FETCH case is clear-cut, here is a patch that normalizes variable fetch sizes in a FETCH command. At a minimum, we can apply this patch. I’ve also added tests in pg_stat_statements utility.sql to demonstrate how queryIds are grouped for the variants of the FETCH statement. -- Sami Imseih Amazon Web Services (AWS)
-
Re: Improve explicit cursor handling in pg_stat_statements
Michael Paquier <michael@paquier.xyz> — 2025-06-03T08:09:18Z
On Mon, Jun 02, 2025 at 02:44:36PM -0500, Sami Imseih wrote: > Since the FETCH case is clear-cut, here is a patch that normalizes variable > fetch sizes in a FETCH command. At a minimum, we can apply this patch. > I’ve also added tests in pg_stat_statements utility.sql to demonstrate > how queryIds > are grouped for the variants of the FETCH statement. { NodeTag type; FetchDirection direction; /* see above */ - long howMany; /* number of rows, or position argument */ + /* number of rows, or position argument */ + long howMany pg_node_attr(query_jumble_ignore); char *portalname; /* name of portal (cursor) */ bool ismove; /* true if MOVE */ + ParseLoc location pg_node_attr(query_jumble_location); } FetchStmt; In terms of silencing the numbers, that's fine by me. Now one issue is that this also masks FETCH_ALL which is a specific keyword in the grammar. Should we offer something more consistent with DeallocateStmt, where we have a boolean flag that would be set when ALL is specified, included in the jumbling? This would mean two separate entries: one for the constants and one for ALL. -- Michael -
Re: Improve explicit cursor handling in pg_stat_statements
Sami Imseih <samimseih@gmail.com> — 2025-06-03T11:40:08Z
> Should we offer something more consistent with DeallocateStmt, where > we have a boolean flag that would be set when ALL is specified, > included in the jumbling? This would mean two separate entries: one > for the constants and one for ALL. Hmm, we could do that to differentiate the keyword ALL. I had a thought earlier about differentiating the other keywords as well: FIRST, LAST, BACKWARD, FORWARD, and ABSOLUTE. Initially, I thought it might be a bit too much, but I do see the merit in this approach, as these are syntactically different from their numeric counterparts. We can accomplish this with just an extra field in FetchStmt, where each of these keywords gets a distinct value. What do you think? -- Sami
-
Re: Improve explicit cursor handling in pg_stat_statements
Sami Imseih <samimseih@gmail.com> — 2025-06-03T18:04:36Z
> Hmm, we could do that to differentiate the keyword ALL. I had a thought > earlier about differentiating the other keywords as well: FIRST, LAST, > BACKWARD, FORWARD, and ABSOLUTE. Initially, I thought it might > be a bit too much, but I do see the merit in this approach, as these are > syntactically different from their numeric counterparts. > > We can accomplish this with just an extra field in FetchStmt, where each > of these keywords gets a distinct value. > > What do you think? v3 is what I'm thinking: In FetchStmt, introduce a new field called explicit_direction (maybe there's a better name?), which is an int that we can assign a value to in gram.c. The value will be 0 if no explicit direction is used. Otherwise, each of the explicit directions (i.e., FIRST, LAST, BACKWARD) will be assigned a unique value. FORWARD ALL and BACKWARD ALL will be treated as distinct from FORWARD and BACKWARD. I considered introducing an enum for these explicit direction values, but didn’t find it particularly useful. If you think it would be beneficial, I can define an enum in parsenodes.h I also slightly reorganized the fetch_args so the like values for n->explicit_direction are nearby each other. It does not change the behavior. Also, modified the tests to match and some additional code comments. -- Sami Imseih Amazon Web Services (AWS)
-
Re: Improve explicit cursor handling in pg_stat_statements
Michael Paquier <michael@paquier.xyz> — 2025-06-04T04:26:10Z
On Tue, Jun 03, 2025 at 01:04:36PM -0500, Sami Imseih wrote: > v3 is what I'm thinking: In FetchStmt, introduce a new field called > explicit_direction (maybe there's a better name?), which is an int that > we can assign a value to in gram.c. The value will be 0 if no explicit > direction is used. Otherwise, each of the explicit directions (i.e., FIRST, > LAST, BACKWARD) will be assigned a unique value. FORWARD ALL and > BACKWARD ALL will be treated as distinct from FORWARD and BACKWARD. Hmm. I was not sure if we'd really need to get down to that, as most of the grammar keywords have the same parsed meaning, but there's a good point with LAST for example that uses a negative value for howMany. If we silence the number, LAST would map with everything else that has FETCH_ABSOLUTE. That would be confusing. > I considered introducing an enum for these explicit direction values, but > didn’t find it particularly useful. If you think it would be beneficial, > I can define an enum in parsenodes.h An enum would shine here IMO, because the value could be self-documented and one would not need to guess what each integer value means. That could be something like a direction_keyword, filled with FETCH_KEYWORD_LAST, FETCH_KEYWORD_BACKWARD, etc. -- Michael
-
Re: Improve explicit cursor handling in pg_stat_statements
Sami Imseih <samimseih@gmail.com> — 2025-06-04T16:51:56Z
> Hmm. I was not sure if we'd really need to get down to that, as most > of the grammar keywords have the same parsed meaning, but there's a > good point with LAST for example that uses a negative value for > howMany. If we silence the number, LAST would map with everything > else that has FETCH_ABSOLUTE. That would be confusing. yes, that is another good case. I think distinguishing between the various keywords makes the most sense. > > I considered introducing an enum for these explicit direction values, but > > didn’t find it particularly useful. If you think it would be beneficial, > > I can define an enum in parsenodes.h > > An enum would shine here IMO, because the value could be > self-documented and one would not need to guess what each integer > value means. That could be something like a direction_keyword, filled > with FETCH_KEYWORD_LAST, FETCH_KEYWORD_BACKWARD, etc. Done in v4. -- Sami
-
Re: Improve explicit cursor handling in pg_stat_statements
Michael Paquier <michael@paquier.xyz> — 2025-06-05T01:00:42Z
On Wed, Jun 04, 2025 at 11:51:56AM -0500, Sami Imseih wrote: >> An enum would shine here IMO, because the value could be >> self-documented and one would not need to guess what each integer >> value means. That could be something like a direction_keyword, filled >> with FETCH_KEYWORD_LAST, FETCH_KEYWORD_BACKWARD, etc. > > Done in v4. Mostly good here, some more comments. + * If an direction_keyword (i.e., FETCH FORWARD) is used, set this field + * to distinguish it from its numeric counterpart (i.e., FETCH 1). This + * value is set only within gram.y. One nitpick comment here is that I would have mentioned that this matters for query jumbling. The tests can be in cursors.sql and not utility.sql, which is the test area for... Cursors. :D FetchDirectionKeywords could be in typedefs.list to avoid the indentation accident with the structure definition. There's no actual practice about that for committers, this note will just serve as a self-reminder once the v19 branch opens for business when I handle this patch. -- Michael
-
Re: Improve explicit cursor handling in pg_stat_statements
Sami Imseih <samimseih@gmail.com> — 2025-06-05T12:42:48Z
> + * If an direction_keyword (i.e., FETCH FORWARD) is used, set this field > + * to distinguish it from its numeric counterpart (i.e., FETCH 1). This > + * value is set only within gram.y. > > One nitpick comment here is that I would have mentioned that this > matters for query jumbling. Done > The tests can be in cursors.sql and not utility.sql, which is the test > area for... Cursors. :D Yeah, not sure how I missed the cursor.sql file. > FetchDirectionKeywords could be in typedefs.list to avoid the > indentation accident with the structure definition. Done. v5 attached. -- Sami
-
Re: Improve explicit cursor handling in pg_stat_statements
Michael Paquier <michael@paquier.xyz> — 2025-06-06T02:06:24Z
On Thu, Jun 05, 2025 at 07:42:48AM -0500, Sami Imseih wrote: > v5 attached. I'm OK with this version, so switching that as ready for committer. -- Michael
-
Re: Improve explicit cursor handling in pg_stat_statements
Sami Imseih <samimseih@gmail.com> — 2025-06-30T12:37:08Z
rebased patch. Regards, Sami
-
Re: Improve explicit cursor handling in pg_stat_statements
Michael Paquier <michael@paquier.xyz> — 2025-07-01T23:42:41Z
On Mon, Jun 30, 2025 at 03:37:08PM +0300, Sami Imseih wrote: > rebased patch. There were two extra pg_stat_statements_reset() calls added to cursors.sql that were not necessary. I have also cut by one the number of FETCH queries with the numbers, as it does not change the coverage outcome, tweaked a bit the comments, and the result looked fine. At the end, applied. -- Michael