Improve helper function checks for consistency and lower overhead #680
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This moves the checking for the presence of helper functions to the collection helper, and relies on an upfront querying of pg_proc, instead of querying for each helper individually. Note that we have to support this both for the main database (for all server-wide data points), as well as for each database to get column/extended statistics information. In passing we also improves the support for checking for different (potentially overloaded) versions of the same functions, by requiring passing the function argument types to the lookup. That is specifically necessary for the multiple historic variants of get_stat_statements that may be present on the same database server, and lets us remove special casing that code.
lfittl
commented
Feb 17, 2025
| return state.PostgresFunction{}, false | ||
| } | ||
|
|
||
| func (s CollectionHelper) HelperExists(name string, inputTypes []string) bool { |
There was a problem hiding this comment.
For context, I plan to add additional behavior here soon that warns the user (and flags the SelfTest logic) in case the function ownership is not as expected (e.g. you created a helper function with the pganalyze user as the owner, instead of a quasi-superuser/etc). To keep reviews easy I've not yet added this here.
lfittl
commented
Feb 19, 2025
| @@ -80,7 +80,12 @@ func runQueryOnDatabase(ctx context.Context, server *state.Server, collectionOpt | |||
| } | |||
| defer db.Close() | |||
|
|
|||
| if postgres.StatsHelperExists(ctx, db, "explain_analyze") { | |||
| h, err := postgres.NewCollectionHelper(ctx, logger, server, collectionOpts, db) | |||
There was a problem hiding this comment.
Maybe consider adding a different constructor or argument here to initialize without the data that's not needed (like Postgres version in this case).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This moves the checking for the presence of helper functions to the collection helper, and relies on an upfront querying of pg_proc, instead of querying for each helper individually. Note that we have to support this both for the main database (for all server-wide data points), as well as for each database to get column/extended statistics information.
In passing this also improves the support for checking for different (potentially overloaded) versions of the same functions, by requiring passing the function argument types to the lookup. That is specifically necessary for the multiple historic variants of get_stat_statements that may be present on the same database server, and lets us remove special casing that code.
On top of #679.