Expose context to notice and notification handlers

[torkelrogstad]

Add new handler types for notices and notifications. These are equivalent to the existing types, but receive a context as the first parameter. This allows for notices and notifications to use values typically stored in contexts, such as logging/tracing metadata.

The existing behavior is kept for backwards compatability, but the OnNotice and OnNotifiation fields of pgconn.Config are deprecated.

---

What do you think of this @jackc? There are parts missing here: documentation and tests. If you think this is a good idea, I'll add those.

[jackc]

They originally didn't take a context as they are called outside of the normal query flow -- possibly even where a context is not available (as you discovered -- I see you had to plumb context down pretty deep in the system).

Do you actually want the context from whatever query happens to be running when the notice or notification is received or do you actually want custom per connection data. This was just brought up in #1746.

[torkelrogstad]

My specific use-case is that I use the context for storing loggers with metadata. The metadata contain entries such as user IDs, RPC endpoint names or test names when running go test. Having this metadata available in the logs from my notice and notification handlers make these logs a lot more useful.

So to answer your question: ideally I want the context related to the query that produced the notice or notification. Since you're bringing it up, is this not what I've coded up here?

[jackc]

So to answer your question: ideally I want the context related to the query that produced the notice or notification. Since you're bringing it up, is this not what I've coded up here?

Not exactly. You are getting the context related to the query that was executing when the notice or notification was received. But there is no guarantee that that query was what produced the notice or notification.

According to the PostgreSQL protocol, notice and notifications can occur at any time. This is easiest to demonstrate with LISTEN / NOTIFY. Have one connection listen on a channel while another connection sends notifications. Have the listening connection perform queries and the notifications will be intermingled. I think it is possible to trigger notices from outside a query as well.

Typically, notices will be related to the running query but there is no guarantee. In addition, there is no guarantee that any query is in progress at all.

[torkelrogstad]

Does that mean it's impossible to be completely sure about the context <-> notification/notice correlation with the current architecture of pgx, or is just a bit complicated?

On the topic of being completely sure: this still has value to me (and I assume others as well), even if it isn't completely accurate. One use case where I'm seeing lots of utility is for output in tests. My current setup for running Go unit tests look like this:

1. Throughout the entire system, use the context to retrieve an appropriate logger. (Specifically, by calling zerolog.Ctx).
2. In tests, use the provided testing.T to create a logger that writes its output to the test logger.
3. From this logger, create a context that's passed to the code under test.

This means I get notification and notice logs that are both:

1. Annotated with the name of the test that's running
2. Tied to the exit code of the test, i.e. non-failing tests produce no output, and failing tests do. This allows me to skip all the DB notices/notifications I don't care about, and only get the ones from failing tests.

For something like this PR to get merged, do you think the context <-> notice/notification correlation must be perfect, or is something like what's in this PR sufficient?

[jackc]

Does that mean it's impossible to be completely sure about the context <-> notification/notice correlation with the current architecture of pgx, or is just a bit complicated?

It's impossible, but not just with the current architecture of pgx, it's impossible at the PostgreSQL protocol level.

---

For your sample use case of test output, that is the type use case I was asking about with regard to instead adding a custom data field to the connection itself. Storing the test / test logger on the connection itself would satisfy this use case.

FWIW, I do a similar thing in my tests to see notices, only I typically create a new connection for each test so I setup the test logger as part of the connect process.

---

For something like this PR to get merged, do you think the context <-> notice/notification correlation must be perfect, or is something like what's in this PR sufficient?

Well, as mentioned above, it actually can't be perfect. The question is if it is worth the potential confusion. We can be almost sure that if we merge this someone is going to file an issue at some point complaining that they are getting notices or notifications connected to the wrong query.

[torkelrogstad]

We can be almost sure that if we merge this someone is going to file an issue at some point complaining that they are getting notices or notifications connected to the wrong query

I think you're right on that! Makes me believe something like this is not a good idea.