chore: bump pgx to v5 #1032
chore: bump pgx to v5 #1032
Conversation
| @@ -315,44 +314,33 @@ func BuildDBConfig(opts *DBConfigOptions) (*pgxpool.Config, error) { | |||
| return nil, err | |||
| } | |||
|
|
|||
| dbconfig.ConnConfig.LogLevel = pgx.LogLevelWarn | |||
There was a problem hiding this comment.
Note to self. Has this log level config been moved to somewhere else?
There was a problem hiding this comment.
Note to self. Has this log level config been moved to somewhere else?
yes it did. instead of a logger within pgx, logging is extracted to be in the pgx tracelog package. I'll have to take a look on how to make that work for us later on.
There was a problem hiding this comment.
Hm, i'd have to build a log adapter for the tracelog package of pgx for our custom logger. 🤔
Something like so:
// this would be an instance of slog logger
logAdapter := log.NewLoggerAdapter(logger)
tracer := &tracelog.TraceLog{
Logger: logAdapter,
LogLevel: tracelog.LogLevelNone,
}
if <pg debug enabled> {
tracer.LogLevel = tracelog.LogLevelDebug
}
conf.ConnConfig.Tracer = tracerthere's readily available adapters for slog already. I don't think it make sense to build a custom logadapter for our custom logger if we agree that slog should be come the default logger and we drop str logger and zap. wdyt?
There was a problem hiding this comment.
I agree that we should move towards adopting slog, but I'm not sure if we should land this PR without a logger in place. One option is that we could shift over to the slog refactor, land that, and then circle back to this PR. If we don't want to do that, I think implementing the custom log adapter would be the way to go, even if it's temporary.
There was a problem hiding this comment.
There is a logger, tegola logger remains untouched by this. PGX however does not come with logger any longer, but expects the user to pass it if required.
I will add a very simple adapter that reflects the previous behaviour of:
dbconfig.ConnConfig.LogLevel = pgx.LogLevelWarn
Which is currently logging anything that pgx considers as warning.
|
I wanted to test the hstore tags and noticed that both in update: update2: |
| @@ -32,6 +32,8 @@ services: | |||
| environment: | |||
| PGUSER: postgres | |||
| PGPASSWORD: postgres | |||
| volumes: | |||
| - ./scripts:/scripts | |||
There was a problem hiding this comment.
this allows us to extend test tables a little more flexible than downloading a dump, wdyt? @ARolek
| break | ||
| // NOTE: if it can also be a tag, then breaking here | ||
| // will never add it to tags | ||
| // break |
There was a problem hiding this comment.
this was an odd find. the comment above explicitly says it can also be a tag, but it will never be parsed to tags with the break in place. Do you remember how that came to be maybe? @ARolek
This was just never noticed, as the case is not tested. 🙈
| return | ||
| } | ||
|
|
||
| log.Warnf("Postgis: %s, %#v", msg, data) |
There was a problem hiding this comment.
I'm having trouble testing this locally. Can you think of an action where pgx would emit a warning before? @ARolek
| Name: "uuid", | ||
| OID: pgtype.UUIDOID, | ||
| }) | ||
| pgxuuid.Register(conn.TypeMap()) |
There was a problem hiding this comment.
I was hoping to avoid by using the builtin pgtype codec for uuid, but I just couldn't get it to work without this external dependency. Considering the official wiki is proposing this approach, I think we're on the safe side tho.
|
tests will pass once #1033 is accepted :) |
description
decipherFields