[*] refactor TryCreateMissingExtensions #1186
[*] refactor TryCreateMissingExtensions #1186gemy26 wants to merge 1 commit intocybertec-postgresql:masterfrom
TryCreateMissingExtensions #1186Conversation
Pull Request Test Coverage Report for Build 21789834250Details
💛 - Coveralls |
0xgouda
added
the
refactoring
|
|
||
| // For security reasons don't allow to execute random strings but check that it's an existing extension | ||
| data, err := QueryMeasurements(ctx, md, sqlAvailable) | ||
| if !md.IsPostgresSource() { |
There was a problem hiding this comment.
if it's not a postgres source, there is no any extensions. we can return immediately
| rows, err := md.Conn.Query(ctx, sqlAvailable, queryArgs...) | ||
| if err != nil { | ||
| log.GetLogger(ctx).Infof("[%s] Failed to get a list of available extensions: %v", md, err) | ||
| l.Infof("[%s] Failed to get a list of available extensions: %v", md, err) |
There was a problem hiding this comment.
that's not how we form logging messages
There was a problem hiding this comment.
Thanks for review,
Could you clarify the correct logging format? , Should I use source-aware logger like:
l.WithField("source", md.Name).Infof("Failed to get a list of available extensions: %v", err)
Or would you prefer a different approach?
| for rows.Next() { | ||
| if err := rows.Scan(&extentionName); err != nil { | ||
| l.Infof("[%s] Failed to scan extension name: %v", md, err) | ||
| continue | ||
| } | ||
| availableExtentions[extentionName] = true |
There was a problem hiding this comment.
I think it's easier to use pgx.CollectRows functionality https://pkg.go.dev/github.com/jackc/pgx/v5#CollectRows
| for _, row := range data { | ||
| availableExts[row["name"].(string)] = true | ||
| // For security reasons don't allow to execute random strings but check that it's an existing extension | ||
| availableExtentions := make(map[string]bool) |
There was a problem hiding this comment.
Why do we need map[string]bool here?
There was a problem hiding this comment.
it's for lookup when checking if extension exist since we check each extension in extensionNames against the available extensions.
| if err != nil { | ||
| log.GetLogger(ctx).Errorf("[%s] Failed to create extension %s (based on --try-create-listed-exts-if-missing input): %v", md, extToCreate, err) | ||
| execArgs := []any{} | ||
| if !md.IsPostgresSource() { |
There was a problem hiding this comment.
If it's not a postgres source, we don't want to create any extensions. It's impossible to create extension in pg_bouncer, for example
pashagolub
left a comment
pashagolub
left a comment
There was a problem hiding this comment.
if we want to refactor we must do it properly
4 participants
TryCreateMissingExtensionsto use direct pgx connection instead of theQueryMeasurementswrapper.