fix: Restore showing LISTEN pgrst in pg_stat_activity

[mkleczek]

DISCLAIMER:
This commit was authored entirely by a human without the assistance of LLMs.

Fixes #4857

Stacked on top of #4860 to enable testing.

[wolfgangwalther]

As a user, I could not care less. I don't think this is important to them. Also, this is not really a thing we're promising to the user.

I'd consider this kind of change + test just a part of #4643, right?

[mkleczek]

As a user, I could not care less. I don't think this is important to them. Also, this is not really a thing we're promising to the user.

I'd consider this kind of change + test just a part of #4643, right?

No, this is a fix of #4857 - it seems we do promise it. https://www.hyrumslaw.com

[wolfgangwalther]

I don't think the fact that Laurence created an issue about my observation that this is not displayed anymore makes this something that we should support as a feature on its own.

[mkleczek]

I don't think the fact that Laurence created an issue about my observation that this is not displayed anymore makes this something that we should support as a feature on its own.

Not sure about that. @steve-chavez would like to revert some features we already merged because of that. Hence the fix.

[steve-chavez]

As a user, I could not care less. I don't think this is important to them. Also, this is not really a thing we're promising to the user.

@wolfgangwalther There's another type of user I put in this diagram on the docs, I called it the "operator" but maybe there are more apt names like "administrator". (I also intended it to point to the db in the diagram, but it came out all crossed). It's expected to monitor the health of PostgREST (and when you do it you have to look at the database queries as well, for things like locked transactions or zombie LISTEN processes).

So #4857 is a bug in the same way #4751 (comment) was (and #4821 follows a similar logic), it's a "deficiency in observability" . PostgREST queries are expected to have a certain shape and so far we have been pretty stable in this regard, since we even have tests:

postgrest/test/spec/Feature/Query/PlanSpec.hs

Line 244 in ea08a4d

aggCol `shouldBe` Just [aesonQQ| "COALESCE((json_agg(ROW(projects.id, projects.name, projects.client_id)) -> 0), 'null'::json)" |]

For this case we should test a basic shape of the LISTENer queries and maybe reword the CHANGELOG.

[steve-chavez]

I think we should agree that some query shape (maybe just the start of a query and wrapping CTEs) should be maintained in minor versions. That does fit with our "no new features" in minor stance. Thoughts?

[wolfgangwalther]

Thanks for clarifying that this was indeed labeled a bug/fix very intentionally. I still disagree, though ;)

I think we should agree that some query shape (maybe just the start of a query and wrapping CTEs) should be maintained in minor versions.

I really think we should not guarantee any of that. The actual queries we are running are really an implementation detail, nothing else. We should not guarantee their shape just because of observability.

TLDR: If you really need a way to identify PostgREST sessions, take the ability to freely change application_name away and force it. Don't let anyone identify PostgREST via its queries. That's just wrong.

Edit: Forgot an important not above...

[steve-chavez]

TLDR: If you really need a way to identify PostgREST sessions, take the ability to freely change application_name away and force it.

That's the problem though. Right now there's no way to differentiate between pool sessions and the LISTEN session. Because we share the same connection uri for both, if you override application_name that wouldn't allow you to pinpoint the LISTEN session (and the connection pool size can be big); all you're left with is the LISTEN query (which is lost as per #4857). I haven't been able to repro this, but in some cases I've seen it's necessary to kill the LISTEN manually with pg_terminate_backend to receive notifications again (maybe #4670 will solve it).

The above gave me an idea #4863.

The actual queries we are running are really an implementation detail, nothing else. We should not guarantee their shape just because of observability.

If we all agree on this we should document it somewhere, maybe on https://docs.postgrest.org/en/v14/#releases.

[laurenceisla]

I don't think the fact that Laurence created an issue about my observation

Ah, sorry, I actually didn't notice your observation, I'd have continued the conversation there instead of making a separate issue.

TLDR: If you really need a way to identify PostgREST sessions, take the ability to freely change application_name away and force it.

The above gave me an idea #4863.

Yeah, agree that changing the application_name is the better solution here. So I think we can close #4857 issue as not-planned in favor of the feature request above?

[steve-chavez]

I really think we should not guarantee any of that. The actual queries we are running are really an implementation detail, nothing else. We should not guarantee their shape just because of observability.

The implementation details of the maintenance queries must also be known currently as argued on #4864.

Yeah, agree that changing the application_name is the better solution here. So I think we can close #4857 issue as not-planned in favor of the feature request above?

Disagree, I think this fix should still go and on the next major we can be explicit that administrators should not rely on queries shapes since they're implementation details. We have to solve both #4863 and #4864 before that announcement IMO.

[postgrest-ci]

Backport failed for v14, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin v14
git worktree add -d .worktree/backport-4859-to-v14 origin/v14
cd .worktree/backport-4859-to-v14
git switch --create backport-4859-to-v14
git cherry-pick -x 04a0e041e421d12f038cf5cd1dbd848e14203825