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

Nr 280084 add support for postgre sql v17 #176

Merged
merged 17 commits into from
Nov 7, 2024

Conversation

rahulreddy15
Copy link
Contributor

Postgres 17 introduced a new table pg_stat_checkpointer
Metrics originally being collected from pg_stat_bgwriter were moved to pg_stat_checkpointer
Queries were modified to collect the metrics from the new table.
Two metrics buffers_backend and buffers_backend_fsync were dropped in Postgres 17. (https://pgpedia.info/p/pg_stat_bgwriter.html) and are no longer available.

The schema and inventory JSONs for the integration tests were modified to reflect the new metric names and modified inventory variables.

Instance definitions were version dependent. A new way to manage version dependence was created in order to future proof.

@rahulreddy15 rahulreddy15 requested review from a team and sigilioso October 29, 2024 11:28
@sigilioso
Copy link
Contributor

Two metrics buffers_backend and buffers_backend_fsync were dropped in Postgres 17. (https://pgpedia.info/p/pg_stat_bgwriter.html) and are no longer available.

From the commit removing the corresponding metrics:

These are actually not checkpointer properties but backend properties.
Also, pg_stat_io provides a more accurate and equivalent report of these
numbers, by tracking all the I/O stats related to backends, including
writes and fsyncs, so storing them in pg_stat_checkpointer was
redundant.

I wonder if we could find some equivalent values in the pg_stat_io table and avoid a breaking change.

Copy link
Contributor

@sigilioso sigilioso left a comment

Choose a reason for hiding this comment

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

Thank you for taking care of this and great job so far!

I'd check if we can get the missing metrics and when that's ready we can sync to update the changelog and prepare the release. In the meantime, find some comments/suggestions below (merely nits).

src/metrics/instance_definitions.go Outdated Show resolved Hide resolved
src/metrics/instance_definitions.go Outdated Show resolved Hide resolved
src/metrics/instance_definitions_test.go Outdated Show resolved Hide resolved
@rahulreddy15
Copy link
Contributor Author

Added a query to get buffers_backend buffers_backend_fsync from pg_stat_io.

All metrics that were reported before postgres v17 are also reported for postgres v17.

Note : Metrics variable names are changed to reflect the table they come from. So:
bgwriter.buffersWrittenForCheckpointsPerSecond -> checkpointer.buffersWrittenForCheckpointsPerSecond
bgwriter.checkpointSyncTimeInMillisecondsPerSecond -> checkpointer.checkpointSyncTimeInMillisecondsPerSecond
bgwriter.checkpointWriteTimeInMillisecondsPerSecond -> checkpointer.checkpointWriteTimeInMillisecondsPerSecond
bgwriter.checkpointsRequestedPerSecond -> checkpointer.checkpointsRequestedPerSecond
bgwriter.checkpointsScheduledPerSecond -> checkpointer.checkpointsScheduledPerSecond
bgwriter.backendFsyncCallsPerSecond -> io.backendFsyncCallsPerSecond
bgwriter.buffersWrittenByBackendPerSecond -> io.buffersWrittenByBackendPerSecond

@sigilioso
Copy link
Contributor

Note : Metrics variable names are changed to reflect the table they come from.

The metrics names change matches the changes in the latest version of PostgreSQL which makes perfect sense. However it is also a breaking change since anything relying on the previous names will stop working for PostgreSQL 17. We can either:

  • Carry on with the breaking changes and notify/document properly.
  • Keep the previous naming and also update the corresponding docs to make the metric source very clear.

@rahulreddy15 rahulreddy15 force-pushed the NR-280084-Add-support-for-PostgreSQL-v17 branch from 8ee389b to 5a71164 Compare October 30, 2024 10:24
Copy link
Contributor

@sigilioso sigilioso left a comment

Choose a reason for hiding this comment

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

The changes LGTM, well done!

There are two small things to take care of:

  • I've left a nit comment regarding the new test.
  • We need to update the CHANGELOG in order that the pre-release and release are automaticall triggered. You'll find more information regarding the changelog in the release-toolkit and coreint-automation repositories. There are also examples in previous PRs. Ping me if you need more context 🙂

src/metrics/instance_definitions_test.go Outdated Show resolved Hide resolved
src/metrics/instance_definitions_test.go Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@sigilioso sigilioso left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@rahulreddy15 rahulreddy15 merged commit 70acad3 into master Nov 7, 2024
15 checks passed
@rahulreddy15 rahulreddy15 deleted the NR-280084-Add-support-for-PostgreSQL-v17 branch November 7, 2024 08:18
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.

Agent 1.57.2 fails to run monitor queries on Postgres 17 on Debian 12
2 participants