Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Postgres 14+: Include toplevel attribute in statement statistics key #463

Merged
merged 2 commits into from
Oct 12, 2023

Conversation

lfittl
Copy link
Member

@lfittl lfittl commented Oct 11, 2023

This could have caused statistics to be incorrect in Postgres 14+ when the same query was called both from within a function (toplevel=false) and directly (toplevel=true), with pg_stat_statements track set to "all".

If affected, the issue may have shown by bogus statistics being recorded, for example very high call counts, since the statement stats diff would not have used the correct reference.

Fix by including toplevel attribute for internal tracking, matching the pgssHashKey struct in pg_stat_statements.c where this data originates.

This could have caused statistics to be incorrect in Postgres 14+ when
the same query was called both from within a function (toplevel=false)
and directly (toplevel=true), with pg_stat_statements track set to "all".

If affected, the issue may have shown by bogus statistics being recorded,
for example very high call counts, since the statement stats diff would
not have used the correct reference.

Fix by including toplevel attribute for internal tracking, matching the
pgssHashKey struct in pg_stat_statements.c where this data originates.
@lfittl lfittl requested review from keiko713 and a team October 11, 2023 03:50
Copy link
Contributor

@keiko713 keiko713 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍

Just wanted to double check:

  • with this, queryid 123 with toplevel true and false stats will be stored (in the collector) separately
  • when a full snapshot is made, we combine two stats data (of toplevel true and false, more precisely, we also combine the same fingerprint queries too) into one query stat

Is above accurate?

@lfittl
Copy link
Member Author

lfittl commented Oct 11, 2023

Looks good 👍

Just wanted to double check:

  • with this, queryid 123 with toplevel true and false stats will be stored (in the collector) separately
  • when a full snapshot is made, we combine two stats data (of toplevel true and false, more precisely, we also combine the same fingerprint queries too) into one query stat

Is above accurate?

Yes, that's correct.

@lfittl lfittl merged commit 9281d71 into main Oct 12, 2023
3 checks passed
@lfittl lfittl deleted the include-toplevel-in-pgss-gkey branch October 12, 2023 02:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants