pgx icon indicating copy to clipboard operation
pgx copied to clipboard

Expose SeverityUnlocalized in Notice

Open mitar opened this issue 1 year ago • 5 comments

Is your feature request related to a problem? Please describe.

I want to log Notices as I get them from the server. Currently they are exposed in Notice struct, but that struct's Severity field comes from localized S field of the PotgreSQL notice message and not V field. This makes is tricky to map severity to log levels.

Describe the solution you'd like

I think ErrorResponseToPgError should just use SeverityUnlocalized instead of Severity if it is set. I cannot imagine localized field is really something people use.

Describe alternatives you've considered

I would have to figure out how can severity values be localized and invert that localization.

Additional context

I do not understand fully why OnNotice gets PgError and not directly ErrorResponse. ErrorResponse has also some nice features like JSON marshal which would be useful for logging. (BTW, ErrorResponse's MarshalJSON does not use idiomatic JSON field name casing. :-( )

I also do not get what exactly I can obtain from PgConn. Documentation says "origin of the notice", but what exactly metadata is available on PgConn to get this origin? It looks pretty opaque struct to me? I do not think I could attach some additional metadata to it in BeforeAcquire?

mitar avatar Mar 27 '24 14:03 mitar

I also do not get what exactly I can obtain from PgConn. Documentation says "origin of the notice", but what exactly metadata is available on PgConn to get this origin? It looks pretty opaque struct to me? I do not think I could attach some additional metadata to it in BeforeAcquire?

Oh, I can use conn.ParameterStatus("application_name") to obtain application name. Awesome!

mitar avatar Mar 27 '24 17:03 mitar

I've added SeverityUnlocalized in a3d9120636fc263debce595f3ba05bcd9f4ee809.

I think ErrorResponseToPgError should just use SeverityUnlocalized instead of Severity if it is set. I cannot imagine localized field is really something people use.

The V field was not previously available. It was only added in PostgreSQL 9.6 which was released in 2016. pgx predates that (and also needed to support 9.5 until 2021).

I do not understand fully why OnNotice gets PgError and not directly ErrorResponse.

As a general design goal I try to avoid the implementation details of the wire protocol from leaking to the higher level layers (not always possible, but usually is). And on a concrete basis, pgproto3.ErrorResponse used to use []bytes that pointed directly to the shared network read buffer for its text fields. So a copy had to be made and a string is much more convenient for the application developer.

Oh, I can use conn.ParameterStatus("application_name") to obtain application name. Awesome!

You can also get the PID of the connection with conn.PID().

jackc avatar Apr 07 '24 14:04 jackc

Awesome, thanks!

You can also get the PID of the connection with conn.PID().

That to me looks less useful because I would have to maintain some weak map between PID and my own data?

mitar avatar Apr 07 '24 16:04 mitar

You can also get the PID of the connection with conn.PID().

That to me looks less useful because I would have to maintain some weak map between PID and my own data?

I think https://github.com/jackc/pgx/issues/1896 would be a perfect solution for this. However, I proposed the idea over a month ago and no one has commented or even thumbs up or thumbs down. 🤷

jackc avatar Apr 10 '24 23:04 jackc

Oh, that is exactly what I wanted. I was looking for where I could put some data. Currently I encode it into application name, but that would be better. I will comment there.

mitar avatar Apr 11 '24 06:04 mitar

Closing as initial request is completed in a3d9120636fc263debce595f3ba05bcd9f4ee809 and further discussion on custom data is on #1896.

jackc avatar May 08 '24 13:05 jackc