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

Drop support for old Postgres versions #343

Merged
merged 8 commits into from
Feb 28, 2023

Conversation

msakrejda
Copy link
Contributor

@msakrejda msakrejda commented Dec 15, 2022

In 06daccd, we dropped CI testing for
Postgres versions older than than 9.6 (which itself has been EOL for over a
year).

Drop support in code to allow us to simplify some logic. Users who
need support for older Postgres releases can use an older collector
for the time being.

@msakrejda msakrejda requested a review from a team December 15, 2022 23:24
@msakrejda msakrejda force-pushed the drop-support-for-old-postgres-versions branch from 23c05ea to fcdc59c Compare December 15, 2022 23:42
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.

ohhh nice! I was wondering about this, thanks for the cleanup ✨
I wonder if we say anything about supported postgres version in somewhere (e.g. README) 🤔

@msakrejda
Copy link
Contributor Author

@keiko713 yeah, I realized we should probably do this when I saw you were accounting for 9.3 in #342. And you're right--it looks like there's some README cleanup (we mention 9.3 there). I'll do that once this gets the green light.

Copy link
Member

@lfittl lfittl 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, with two minor comments.

Overall, I think dropping 9.5 and older support is clearly the right choice - those versions are long gone.

9.6 and 10 there are still some late upgraders, so doesn't hurt to keep them around for a bit longer and then drop them when we either have a good reason, or more time has passed.

// MinRequiredPostgresVersion - We require PostgreSQL 9.3 or newer
MinRequiredPostgresVersion = PostgresVersion93
// MinRequiredPostgresVersion - We require PostgreSQL 9.6 or newer
MinRequiredPostgresVersion = PostgresVersion96
Copy link
Member

Choose a reason for hiding this comment

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

There is a hard-coded "9.3 or newer is required." message on the top of input/full.go that also needs to be updated.

Copy link
Member

Choose a reason for hiding this comment

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

Re-using this thread - On another note, I just realized we should probably raise this to Postgres 10 being the minimum - because that was the first version to use $n style replacement characters in pg_stat_statements.

See #383, which will effectively make Postgres 9.6 not work with the collector (since we fail to parse / normalize the statements containing "?").

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. I'll update once #339 lands.

input/postgres/backends.go Show resolved Hide resolved
@msakrejda
Copy link
Contributor Author

I pushed an update cleaning up other mentions of older versions. The last commit gets rid of support for parsing pre-9.6 log lines (at least those that were documented as such). Let me know if we should be more conservative and I can just drop that commit, but I checked 9.6 xlog.c and vacuumlazy.c, and those parts of the messages are always present.

Copy link
Member

@lfittl lfittl left a comment

Choose a reason for hiding this comment

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

Thanks for cleaning that up!

state/postgres_statement.go Outdated Show resolved Hide resolved
state/postgres_statement.go Show resolved Hide resolved
logs/analyze.go Show resolved Hide resolved
@msakrejda
Copy link
Contributor Author

I think this is ready to go now, but I can wait until #339 and #342 land to avoid causing unnecessary conflicts.

In 06daccd, we dropped CI testing for
Postgres versions older than 9.6 (which itself has been EOL for over a
year).

Drop support in code to allow us to simplify some logic. Users who
need support for older Postgres releases can use an older collector
for the time being.
@msakrejda msakrejda force-pushed the drop-support-for-old-postgres-versions branch from 3641b0c to 6ea8076 Compare February 28, 2023 01:51
README.md Show resolved Hide resolved
Copy link
Member

@lfittl lfittl left a comment

Choose a reason for hiding this comment

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

👍 Re-reviewed and looks good

@msakrejda msakrejda merged commit 38a247a into main Feb 28, 2023
@msakrejda msakrejda deleted the drop-support-for-old-postgres-versions branch February 28, 2023 02:18
msakrejda added a commit that referenced this pull request Mar 1, 2023
This was accidentally mangled a bit in #343.
@msakrejda msakrejda mentioned this pull request Mar 1, 2023
msakrejda added a commit that referenced this pull request Mar 1, 2023
This was accidentally mangled a bit in #343.
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.

3 participants