fix(inputs.postgresql_extensible): Retry database version detection before failing - #19367
Conversation
|
Thanks so much for the pull request! |
telegraf-tiger
Bot
added
fix
|
!signed-cla |
3e9b1d6 to
e13ce2b
Compare
| var dbVersion int | ||
| if err := p.service.DB.QueryRow(query).Scan(&dbVersion); err != nil { | ||
| dbVersion = 0 | ||
| // Surface the error instead of proceeding with an unknown version (0), |
There was a problem hiding this comment.
it should try again here. we see that version error query happens only when underneath connection was closed without database/sql noticing it. Error here will force connection to close, so new connection will be established on retry.
Alternative proposed in the ticket is to switch pool from database/sql to pgx pool: latter does internal connection healtcheck before running queries
There was a problem hiding this comment.
Good call, thanks. Switched to retrying the version query once on failure so a stale pooled connection gets dropped and a fresh one is used, and only error out if the retry also fails. Left the pgx-pool switch out of scope for this fix.
…efore failing The per-Gather server-version lookup swallowed any error and fell back to version 0, which then selected the wrong set of version-gated queries and produced confusing "column ... does not exist" failures (e.g. when a pooled connection was closed without database/sql noticing). Retry the version query once on failure so the stale connection is dropped and a fresh one is used, and only return the error if the retry also fails, instead of silently running version-mismatched queries. Add regression tests for both paths. This also surfaced two integration tests that connected to a bare localhost database and only passed because the error was swallowed; convert them to the existing testcontainer helper so they run against a real database. Fixes influxdata#18409.
e13ce2b to
f48fabb
Compare
DoTuanAnh2k1
changed the title
|
Download PR build artifacts for linux_amd64.tar.gz, darwin_arm64.tar.gz, and windows_amd64.zip. 📦 Click here to get additional PR build artifactsArtifact URLs |
|
@DoTuanAnh2k1 please restore the PR description template as we cannot review your contribution otherwise! |
|
@srebhan done — restored the PR description template. Thanks! |
srebhan
changed the title
srebhan
left a comment
srebhan
left a comment
There was a problem hiding this comment.
Thanks @DoTuanAnh2k1 for your contribution! Some minor comments from my side below...
| func TestPostgresqlSqlScript(t *testing.T) { | ||
| q := []query{{ | ||
| Script: "testdata/test.sql", | ||
| MinVersion: 901, | ||
| Withdbname: false, | ||
| Tagvalue: "", | ||
| }} | ||
|
|
||
| addr := fmt.Sprintf( | ||
| "host=%s user=postgres sslmode=disable", | ||
| testutil.GetLocalHost(), | ||
| ) | ||
|
|
||
| p := &Postgresql{ | ||
| Log: testutil.Logger{}, | ||
| Config: postgresql.Config{ | ||
| Address: config.NewSecret([]byte(addr)), | ||
| IsPgBouncer: false, | ||
| }, | ||
| Databases: []string{"postgres"}, | ||
| Query: q, | ||
| func TestPostgresqlSqlScriptIntegration(t *testing.T) { | ||
| if testing.Short() { | ||
| t.Skip("Skipping integration test in short mode") | ||
| } | ||
| require.NoError(t, p.Init()) | ||
|
|
||
| var acc testutil.Accumulator | ||
| require.NoError(t, p.Start(&acc)) | ||
| defer p.Stop() | ||
| require.NoError(t, acc.GatherError(p.Gather)) | ||
| queryRunner(t, []query{{ | ||
| Script: "testdata/test.sql", | ||
| MinVersion: 901, | ||
| }}) | ||
| } |
There was a problem hiding this comment.
Please revert this change! This has nothing to do with what the PR title says. If you want to fix tests, please do so in a separate PR!
| addr := fmt.Sprintf( | ||
| "host=%s user=postgres sslmode=disable", | ||
| testutil.GetLocalHost(), | ||
| ) | ||
|
|
||
| p := &Postgresql{ | ||
| Log: testutil.Logger{}, | ||
| Config: postgresql.Config{ | ||
| Address: config.NewSecret([]byte(addr)), | ||
| }, | ||
| } | ||
| require.NoError(t, p.Init()) | ||
|
|
||
| var acc testutil.Accumulator | ||
| require.NoError(t, p.Start(&acc)) | ||
| defer p.Stop() | ||
| require.NoError(t, acc.GatherError(p.Gather)) | ||
| // `pg_stat_database` includes `stats_reset`, one of the ignored columns. | ||
| acc := queryRunner(t, []query{{ | ||
| Sqlquery: "select * from pg_stat_database", | ||
| MinVersion: 901, | ||
| }}) |
There was a problem hiding this comment.
Please revert this change! This has nothing to do with what the PR title says. If you want to fix tests, please do so in a separate PR!
| mock.ExpectQuery("server_version_num"). | ||
| WillReturnRows(sqlmock.NewRows([]string{"version"}).AddRow(1400)) |
There was a problem hiding this comment.
| mock.ExpectQuery("server_version_num"). | |
| WillReturnRows(sqlmock.NewRows([]string{"version"}).AddRow(1400)) | |
| mock.ExpectQuery("server_version_num").WillReturnRows(sqlmock.NewRows([]string{"version"}).AddRow(1400)) |
|
Hello! I am closing this issue due to inactivity. I hope you were able to resolve your problem, if not please try posting this question in our Community Slack or Community Forums or provide additional details in this issue and reqeust that it be re-opened. Thank you! |
3 participants
Summary
postgresql_extensibleruns a server-version query at the start of everyGatherto decide which version-gated queries to run. The error from that query was silently swallowed, defaulting the version to0:With version
0themin_version/max_versiongating selects the wrong queries, so a query intended for another version gets executed and fails with a confusing error such ascolumn p.num_dead_tuples does not exist(#18409). This happens when a pooled connection is closed withoutdatabase/sqlnoticing.The version query is now retried once on failure — dropping the stale connection so a fresh one is used (per @redbaron's review) — and the error is only returned if the retry also fails, instead of silently running version-mismatched queries. Two integration tests that connected to a bare localhost database (and only passed because the error was swallowed) are converted to the existing testcontainer helper so they run against a real database.
Checklist
Related issues
resolves #18409