add: zero-downtime restarts with SIGHUP

[mkleczek]

Provides a way to implement zero-downtime upgrades by letting PostgREST start a replacement process and hand traffic over before the old process exits.

The idea is to Install a SIGHUP handler that requests a restart. It starts the current executable again, waits until the replacement reaches the application ready point, commits the handover, and then stops the old server.

Enable the SIGHUP restart handler only when server-reuseport is enabled and both the main and admin servers use TCP sockets. This keeps restart enabled only for configurations where the replacement can bind its listening sockets before the parent shuts down.

Restart process is integrated with systemd notify by reporting RELOADING=1 during restart and then updating MAINPID together with READY=1 once the replacement process is ready.

---

Implemented control flow:

Standalone Startup

1. runRestartable starts.
2. It checks PGRST_HANDOVER_READ_FD and PGRST_HANDOVER_WRITE_FD.
3. No handover fds are present, so mode is standalone.
4. App.run starts the admin server immediately.
5. App.run installs normal signal handlers.
6. App.run starts the PostgreSQL listener.
7. App.run loads the schema cache.
8. App.run binds the main API socket.
9. Warp enters beforeMainLoop.
   1. PostgREST records the main socket as live.
   2. ready closeSockets ... is called.
   3. If restart is enabled, the SIGHUP handler is installed.
   4. Process is now serving API and admin traffic.

Restart Request

1. The old process receives SIGHUP.
2. The SIGHUP handler calls requestReplacement.
3. The old process sends RELOADING=1 to systemd, if NOTIFY_SOCKET is present.
4. The old process creates two pipes for the private handover channel.
5. The old process forks.
6. The child execs replacementExecutable with the same arguments and environment plus the handover fd env vars.
7. The old process waits for READY from the child.

Replacement Startup

1. The new process starts.
2. runRestartable sees the handover fd env vars.
3. Mode is replacement.
4. App.run does not start the admin server immediately.
5. App.run installs normal signal handlers.
6. App.run starts the PostgreSQL listener.
7. App.run loads the schema cache.
   • If add: load schema cache dump from URI at startup #5031 is merged it can be loaded from parent admin server - even with SO_REUSEPORT there is no risk because replacement admin server is not started yet.
8. App.run binds the main API socket.
9. Warp enters beforeMainLoop.
   1. PostgREST records the main socket as live.
   2. Because mode is replacement, the admin server is started now.
   3. ready closeSockets ... is called.
   4. The new process writes READY to the old process.
   5. The new process blocks waiting for COMMIT.

Commit

1. The old process receives READY.
2. The old process sends MAINPID= and READY=1 to systemd.
3. The old process writes COMMIT to the new process.
4. The old process runs stopAction, which closes old API/admin sockets.
5. Old Warp stops accepting new connections and drains in-progress work.
6. The new process receives COMMIT.
7. The new process installs its own SIGHUP handler if restart is enabled.
8. The new process closes the handover channel.
9. The new process is now the active process.

Failure Before Commit

1. If the new process exits or closes the handover channel before READY, the old process reports HandoverFailed.
2. The old process keeps serving.
3. If needed, the old process terminates the replacement child.
4. No COMMIT is sent.
5. Old sockets remain open, so handover does not create downtime.

[steve-chavez]

@mkleczek Can you explain what's the difference between this and #5036?

[mkleczek]

@mkleczek Can you explain what's the difference between this and #5036?

This one is based on #5036 and automates the whole process inside PostgREST itself (making #5036 obsolete).

With this PR, user is able to restart (without downtime!) PostgREST. So upgrade would be:

1. copy the new binary overwriting the old one
2. issue kill -HUP <pid>

It also cooperates with systemd so you can add the following to the service file:

...
NotifyAccess=main
ExecReload=/bin/kill -HUP $MAINPID
...

to have zero-downtime restarts (so you can upgrade but also change any configuration value).

[steve-chavez]

It also cooperates with systemd

We also have #1517 asking for systemd integration.

Looks very interesting but there's a non-trivial amount of code added and to be reviewed.

Perhaps we can reuse this package https://hackage.haskell.org/package/systemd-2.3.0 somehow? Or is there a way to introduce systemd integration in a more gradual way?

[steve-chavez]

We should also update this doc https://docs.postgrest.org/en/v14/integrations/systemd.html with this feature.

[mkleczek]

It also cooperates with systemd

We also have #1517 asking for systemd integration.

Looks very interesting but there's a non-trivial amount of code added and to be reviewed.

Perhaps we can reuse this package https://hackage.haskell.org/package/systemd-2.3.0 somehow? Or is there a way to introduce systemd integration in a more gradual way?

I wasn't aware of systemd package - looks like it can indeed be re-used.

Having said that, the goal of this PR is not systemd integration by itself, it is here only because restart without notifying systemd about new main process will interfere with systemd managing it. So I implemented basic systemd notification support.

[mkleczek]

Perhaps we can reuse this package https://hackage.haskell.org/package/systemd-2.3.0 somehow? Or is there a way to introduce systemd integration in a more gradual way?

I wasn't aware of systemd package - looks like it can indeed be re-used.

Having said that, the goal of this PR is not systemd integration by itself, it is here only because restart without notifying systemd about new main process will interfere with systemd managing it. So I implemented basic systemd notification support.

[mkleczek]

Looks very interesting but there's a non-trivial amount of code added and to be reviewed.

The most difficulty was not systemd integration as such but with coming up with the right startup sequence and coordination between the parent and child process so that this feature, SO_REUSEPORT and (once merged) #5031 can seamlessly work together.

[steve-chavez]

I wasn't aware of systemd package - looks like it can indeed be re-used.

Let's try to do that and see how much code reduction can we get.

The most difficulty was not systemd integration as such but with coming up with the right startup sequence and coordination between the parent and child process so that this feature SO_REUSEPORT and (once merged) #5031 can seamlessly work together.

It'd be easier to read some docs about the above behavior before trying to review code here, something like a sequence diagram could help. Otherwise it's not easy to understand the logic.

[steve-chavez]

Also check if https://github.com/hercules-ci/warp-systemd helps. TBH I never understood why is systemd socket activation not enough for zero-downtime upgrades for our case. Seems this doesn't need SO_REUSEPORT interaction too.

[mkleczek]

Also check if https://github.com/hercules-ci/warp-systemd helps. TBH I never understood why is systemd socket activation not enough for zero-downtime upgrades for our case. Seems this doesn't need SO_REUSEPORT interaction too.

Systemd socket activation is not zero-downtime - there is downtime period between old instance stopping listening and new instance starting accepting connections. New requests are queued during this period and the clients either:

• see a spike in request latency when all goes well
• if startup time is longer for some reason (or the new instance cannot start) clients will experience timeouts

To achieve real zero-downtime we need multiple instances running at the same time, so that the new instance is handling traffic before the old one stops listening and gracefully shuts down.
That can be achieved only with some kind of a load balancing proxy in front of PostgREST: SO_REUSEPORT is such a (kernel level) proxy.

Secondly, systemd socket activation is Linux only. This PR implements it on all Posix compliant systems (systemd notifications are optional - lack of systemd environment does not prevent the feature to work).

And last but not least: from the point of view of operations or devops teams, having it implemented OOTB in PostgREST simplifies their lives a lot: no need to implement/maintain additional, custom, environment/OS specific scripts/configurations.

[mkleczek]

I wasn't aware of systemd package - looks like it can indeed be re-used.

Let's try to do that and see how much code reduction can we get.

On the second thought - I am not convinced it is worth it. We are talking only a single function, 30 lines of code:

withSystemdNotifier :: (SystemdNotifier -> IO a) -> IO a
withSystemdNotifier action =
  lookupEnv "NOTIFY_SOCKET" >>= maybe (action mempty) withNotifySocket
  where
    withNotifySocket notifySocket =
      bracket
        (openNotifySocket notifySocket)
        NS.close
        $ \sock ->
          action $ NSB.sendAll sock . renderSystemdNotifications

    openNotifySocket notifySocket =
      bracketOnError
        (NS.socket NS.AF_UNIX NS.Datagram NS.defaultProtocol)
        NS.close
        $ \sock ->
          NS.connect sock (NS.SockAddrUnix $ notifySocketAddress notifySocket) $> sock

    notifySocketAddress ('@':xs) = '\0' : xs
    notifySocketAddress xs       = xs

    renderSystemdNotifications =
      ensureTrailingNewline . BS.intercalate "\n" . toList . fmap render
      where
        render = \case
          NotifyReady        -> "READY=1"
          NotifyReloading    -> "RELOADING=1"
          NotifyMainPid pid  -> "MAINPID=" <> showByteString (fromIntegral pid :: Int)

    ensureTrailingNewline txt
      | "\n" `BS.isSuffixOf` txt = txt
      | otherwise                = txt <> "\n"

The most difficulty was not systemd integration as such but with coming up with the right startup sequence and coordination between the parent and child process so that this feature SO_REUSEPORT and (once merged) #5031 can seamlessly work together.

It'd be easier to read some docs about the above behavior before trying to review code here, something like a sequence diagram could help. Otherwise it's not easy to understand the logic.

See updated PR description.

[steve-chavez]

This PR implements it on all Posix compliant systems (systemd notifications are optional - lack of systemd environment does not prevent the feature to work).

@mkleczek I wasn't aware of that at all, could you add some user-facing docs to better understand?

[steve-chavez]

This separation of src/library-windows/PostgREST/Process/Restart/Impl.hs and src/library-posix/.. does help a lot in reviewing.

I'm not sure about the Impl.hs naming though. @taimoorzaeem Perhaps you have other suggestions?

[steve-chavez]

On the second thought - I am not convinced it is worth it. We are talking only a single function, 30 lines of code

Now that the code is better organized into modules it looks easier to review, so not adamant on reusing a library anymore.

[steve-chavez]

Why is systemd inside the library-posix, shouldn't that be in another module?

[mkleczek]

Why is systemd inside the library-posix, shouldn't that be in another module?

It was just too small of a function (and used in only one place) to extract it to a separate module. I'm open to do that if you find it necessary/useful.

[taimoorzaeem]

Hmm, I don't understand why we need the whole library-posix/ and library-windows separation when all we need to do is just add one preprocessor directive. And then move this module to library/PostgREST.

I haven't yet seen any haskell library do this kind of separation so it seems odd to me. Using the CPP directives is the idiomatic haskell way, unless implementation differences are vast, which doesn't seem to be the case here.

Suggested change

	runRestartable ::
	ReplacementConfig ->
	AppRun a ->
	IO a
	runRestartable replacementCfg runApp = do
	bracketOnError
	getChildControl
	(traverse_ closeDuplexChannel) $
	\childControl -> do
	handoverLock <- newMVar ()
	runApp
	(HandoverMode $ isJust childControl)
	(ready replacementCfg childControl handoverLock)
	runRestartable ::
	ReplacementConfig ->
	AppRun a ->
	IO a
	#ifndef mingw32_HOST_OS
	runRestartable replacementCfg runApp = do
	bracketOnError
	getChildControl
	(traverse_ closeDuplexChannel) $
	\childControl -> do
	handoverLock <- newMVar ()
	runApp
	(HandoverMode $ isJust childControl)
	(ready replacementCfg childControl handoverLock)
	#else
	runRestartable _ runApp =
	runApp (HandoverMode False) readyUnsupported
	where
	readyUnsupported _ withRequestReplacement =
	withRequestReplacement $ throwIO $ HandoverFailed "Restart handover is not supported on this platform."
	#endif

[mkleczek]

Hmm, I don't understand why we need the whole library-posix/ and library-windows separation when all we need to do is just add one preprocessor directive.

Preprocessor directives and conditional compilation are evil :)

The differences are more than one function - there are differences in imports as well (unix library is only available on non-windows).

I haven't yet seen any haskell library do this kind of separation so it seems odd to me. Using the CPP directives is the idiomatic haskell way, unless implementation differences are vast, which doesn't seem to be the case here.

I find this way of separating platform specific code much more readable and principled - instead of ad-hoc text inclusion/exclusion that quickly becomes unreadable mess, you have clear separation and visibility into what's platform neutral, what's platform specific and what the platform interface is.

But I am open to changing it to conditional compilation if that's preferred way.

@steve-chavez @wolfgangwalther WDYT?

[wolfgangwalther]

Conditional compilation has other problems, I believe. Some tooling doesn't work with it nicely. I remember doctests were a problem, but maybe not anymore since they are now changed to run compiled code anyway. I think there was something else as well, but I can't remember what it was.

I like the approach introduced here - but I think we should follow through on it. How about using the same pattern for the existing PostgREST.Unix module - which is a historic relict anyway... now that Windows supports Unix Sockets...

We should probably introduce this pattern for existing code in a separate PR and have the discussion there.

[steve-chavez]

I remember the sample bash script that was added on #5036 before and it was a few lines.

Frankly, the cost of of maintaining that bash script is looking much smaller than maintaining this amount of code in core.

And last but not least: from the point of view of operations or devops teams, having it implemented OOTB in PostgREST simplifies their lives a lot: no need to implement/maintain additional, custom, environment/OS specific scripts/configurations.

Yeah but then that's shifting a higher maintenance cost on us, I see some mention of windows being unimplemented in the code; that opens the door for users demanding we implement that later too and deal with edge cases.

Secondly, systemd socket activation is Linux only. This PR implements it on all Posix compliant systems (systemd notifications are optional - lack of systemd environment does not prevent the feature to work).

I don't think we need that kind of flexibilty, we only need systemd.

I'll let other chime in but as it is now this is looking like too much to review and not the right design.

[mkleczek]

I remember the sample bash script that was added on #5036 before and it was a few lines.

Yeah, this PR root is actually your suggestion here: #4703 (comment)

I think SIGHUP handling is a very useful feature in servers (such as haproxy or Nginx).

Frankly, the cost of of maintaining that bash script is looking much smaller than maintaining this amount of code in core.

We already pay the cost of maintaining in-process configuration reloading even though it is imperfect and does not provide full reloading (eg. it is not possible to change log level without restart). It will also never provide a way to turn on GHC metrics in runtime as it requires restart.

So one might think of this PR as the ultimate solution for configuration (and schema cache) reloading. As a bonus it allows upgrades.
The enabler was #4703.

Yeah but then that's shifting a higher maintenance cost on us, I see some mention of windows being unimplemented in the code; that opens the door for users demanding we implement that later too and deal with edge cases.

True.

Secondly, systemd socket activation is Linux only. This PR implements it on all Posix compliant systems (systemd notifications are optional - lack of systemd environment does not prevent the feature to work).

I don't think we need that kind of flexibilty, we only need systemd.

I don't understand this. This PR is more complex because it implements integration with systemd - without that it would be much simpler (but would not work properly under systemd).

I'll let other chime in but as it is now this is looking like too much to review and not the right design.

I hear you - let's think about the idea some more and get back to it in the future.

[wolfgangwalther]

First of all, the idea and feature is fantastic. I believe we should most certainly have this, if we can do it in a maintainable and understandable way.

I'm not sure whether that works, but one thing that should be pretty maintainable, I believe, would be to separate this "restart yourself + handover to new process" into a generic library, one that is not PostgREST-specific. One way to do this would be to create an entirely separate Haskell/hackage project, but I believe this would not make it very maintainable in other aspects for us. However, if we can create this generic library as a sub-library in the postgrest.cabal file, have the entire generic code for it isolated and have some rather simple calls from PostgREST into this library to define the various PostgREST specific things at each step, that could work.

It would allow us to review, understand and maintain the two different complexities involved here separately: the restart/replacement/handover process itself vs. the handling of (admin) sockets, live/ready state etc.

---

I took the description of the implementation of the PR body and stripped it of everything specific to PostgREST - so the generic algorithm would be:

Standalone Startup

Standalone Startup

1. runRestartable starts.
2. It checks ***_HANDOVER_READ_FD and ***_HANDOVER_WRITE_FD.
3. No handover fds are present, so mode is standalone.
4. [App starts...]
5. If restart is enabled, the SIGHUP handler is installed.

Restart Request

1. The old process receives SIGHUP.
2. The SIGHUP handler calls requestReplacement.
3. The old process sends RELOADING=1 to systemd, if NOTIFY_SOCKET is present.
4. The old process creates two pipes for the private handover channel.
5. The old process forks.
6. The child execs replacementExecutable with the same arguments and environment plus the handover fd env vars.
7. The old process waits for READY from the child.

Replacement Startup

1. The new process starts.
2. runRestartable sees the handover fd env vars.
3. Mode is replacement.
4. [App starts in a different way than in standalone mode...]
5. The new process writes READY to the old process.
6. The new process blocks waiting for COMMIT.

Commit

1. The old process receives READY.
2. The old process sends MAINPID= and READY=1 to systemd.
3. The old process writes COMMIT to the new process.
4. The old process runs stopAction [...].
5. The new process receives COMMIT.
6. The new process installs its own SIGHUP handler if restart is enabled.
7. The new process closes the handover channel.
8. The new process is now the active process.

Failure Before Commit

1. If the new process exits or closes the handover channel before READY, the old process reports HandoverFailed.
2. The old process keeps [running].
3. If needed, the old process terminates the replacement child.
4. No COMMIT is sent.

---

A lot of this seems very generic to me. I have not looked at the code at all, but I would expect the generic interface to be something roughly like:

1. Call runRestartable standaloneCallback restartCallback stopCallback, providing three functions which contain all the application specific flow of standalone and restart modes as well as when stopping.
2. The standaloneCallback and restartCallback functions take (at least) a single argument in which the restarter passes a ready or done function or so. For the standalone case, calling this function would register the SIGHUP handler. For the restart case, it would send READY and block for COMMIT.

Having a very simple executable compiled as a test-case for this generic library and run some simple tests with it, would be great - we could even test systemd integration via NixOS tests.

On the PostgREST-side we should be able to see the difference between the standalone and restart callbacks easily, to check on the PostgREST-specific logic here.

[mkleczek]

First of all, the idea and feature is fantastic. I believe we should most certainly have this, if we can do it in a maintainable and understandable way.

I'm not sure whether that works, but one thing that should be pretty maintainable, I believe, would be to separate this "restart yourself + handover to new process" into a generic library,

It is structured that way - there is a generic Restart module implementing the whole workflow. PostgREST App module just calls runRestartable function providing a callback.

There should be no problem with moving the generic module to a separate library.

[steve-chavez]

The PR consists of several standalone commits to make it easier for reviewers. The assumption is that review is done commit by commit, not the whole PR.

I didn't want to split it into several PRs because adding a library that is not used anywhere is smelly - so I decided to raise a single PR as a standalone feature consisting of multiple separate commits.

As an example postgresql AIO system was introduced in several commits that didn't had any use in the server (mentioned before) plus it spawned several mailing list threads.

We're also limited by github collapsing multiple comments on the same PR and one has to click on the UI to expand and find some comment. So IMO there's nothing "smelly" about doing multiple PRs (with still unused code), on the contrary it helps us review.

[wolfgangwalther]

I don't have the time to review it right now, but still wanted to drop a comment here about the process:

The PR consists of several standalone commits to make it easier for reviewers. The assumption is that review is done commit by commit, not the whole PR.
I didn't want to split it into several PRs because adding a library that is not used anywhere is smelly - so I decided to raise a single PR as a standalone feature consisting of multiple separate commits.

I agree 100%. It makes no sense to me to introduce something that is not used. I always review commit by commit, so this should be fine.

[steve-chavez]

We're also limited by github collapsing multiple comments on the same PR and one has to click on the UI to expand and find some comment.

Also note that it's not only about the comments but unresolved threads/feedback also get collapsed on github UI, a problem with this before: #4703 (comment).

I agree 100%. It makes no sense to me to introduce something that is not used.

The process of introducing sublibraries into postgREST is new and I'd consider "used" as something that is "tested" and we're doing exactly that. It's possible that someone asks us later to publish some sublibrary to Hackage too.

I think we should do this new process right and make it easier for all of us to review; it's certainly not easier for me. Maybe @taimoorzaeem can also chime in.

[mkleczek]

We're also limited by github collapsing multiple comments on the same PR and one has to click on the UI to expand and find some comment. So IMO there's nothing "smelly" about doing multiple PRs (with still unused code), on the contrary it helps us review.

Fine for me, will open a new PR for the library and its tests then.

[mkleczek]

So IMO there's nothing "smelly" about doing multiple PRs (with still unused code), on the contrary it helps us review.

See #5139

[taimoorzaeem]

The process of introducing sublibraries into postgREST is new and I'd consider "used" as something that is "tested" and we're doing exactly that. It's possible that someone asks us later to publish some sublibrary to Hackage too.

I think we should do this new process right and make it easier for all of us to review; it's certainly not easier for me. Maybe @taimoorzaeem can also chime in.

I agree that it's difficult to review huge PRs, and multiple PRs would be nice, but I am also not in favor of having unused code in codebase, unless it's the only way to have the feature. Not sure what's the most effective way to deal with this yet.

One possible direction could be to develop process-restart as a separate library project, within our codebase, like it's own process-restart.cabal file and write tests etc. But only merge the whole thing when it's integrated into PostgREST. That might make the reviewing process slightly less hard?

[mkleczek]

One possible direction could be to develop process-restart as a separate library project, within our codebase, like it's own process-restart.cabal file and write tests etc. But only merge the whole thing when it's integrated into PostgREST. That might make the reviewing process slightly less hard?

I created a separate library in postgest.cabal. Having a separate cabal project confuses tooling (HLS cannot deal with this well).

See #5139

[steve-chavez]

I agree that it's difficult to review huge PRs, and multiple PRs would be nice, but I am also not in favor of having unused code in codebase, unless it's the only way to have the feature

It looks like github is pushing projects to use stacked PRs. They mention:

Merge one, some, or all by landing an entire stack altogether or individual layers one at a time.

So perhaps we can take advantage of that to make sure we don't merge unused code? I'm assuming there's a way to enforce the merge all option.

That would be the best outcome, with that we don't get long PR threads with collapsed content that make reviewing harder.

[taimoorzaeem]

It looks like github is pushing projects to use stacked PRs.

...

That would be the best outcome, with that we don't get long PR threads with collapsed content that make reviewing harder.

Hm agree, We should definitely try out stacked PRs and see if it makes the reviewing process easier.

[steve-chavez]

I'm trying to make the new stacked PRs work following: https://docs.github.com/en/pull-requests/how-tos/create-pull-requests/creating-stacked-pull-requests#creating-a-stack-from-the-github-website.

So far it looks like it only works for branches on the upstream repo, which is a big disadvantage.

[steve-chavez]

Confirming the above:

Stacked pull requests require all branches to be in the same repository. Cross-fork stacks are not supported.
https://docs.github.com/en/pull-requests/get-started/about-stacked-prs#where-can-you-use-stacked-pull-requests

Since all branches have to be in the same repo, I cannot open a PR to upstream postgREST because when targeting my own repo/branch I don't get that option on the UI.

I haven't tried with CLI yet.

Edit: wasted a lot of time with gh stack CLI, I couldn't get it to work.

[wolfgangwalther]

While reviewing #5139, I came up with this comment. Not a full review of this PR.

[wolfgangwalther]

The test executable and the library call this mode. The type is called HandoverMode, I believe.

We should use the same term throughout. At least \mode in both cases, but possibly handoverMode everywhere? Or RestartMode?

I didn't think too hard about which term fits the best, but different terms are confusing.

(rant3: this comment is about both PRs, but this line is only available in this PR. So I need to add the feedback in one PR, even though it is mainly about the other... did I already say that splitting this up into multiple PRs is bad?)

[steve-chavez]

did I already say that splitting this up into multiple PRs is bad?

I'm confused as to why we're reviewing thinking on this PR and not only on #5139. The way I understood this new process of sublibraries to work, is to come up with a generic library with its own subtests independent of PostgREST.

If somehow the generic library is not enough when integrating into PostgREST, it can be changed on a later PR but first we should ensure the library can stand on its own and that's what #5139 is about -- establish the generic foundation.

[wolfgangwalther]

Just because the library is generic, it does not mean that it is entirely independent. It is used by PostgREST - that's its only purpose. So at the minimum, it needs to satisfy PostgREST's requirements. It's pointless to review something that "works on its own", if you don't look at "does it work for PostgREST, too?" at the same time.

As argued in the other PR "generic library" should not be misunderstood as "generally useful for others". That's not the point of splitting this up. My primary motivation is ease of maintenance and testability: A single entrypoint into this generic piece of code and one that can be used independently in a separate executable for a simplified E2E test (just a regular Haskell module with some hspec test won't suffice here).

[steve-chavez]

I'm still unsure if this is the right process. Let's compare #4984, which merged a new external dependency (aeson-jsonpath) and was a smooth high-level review process for the most part. In comparison for #5139, we need to ask to add for a README or tests, things that are a given on a hackage package. ISTM that maximizes ownership and pushes for higher quality, pushing to hackage would take longer, but it's not that merging #5139 is going to be much faster since there's still a good amount of code we haven't looked at.

So my thinking is, why not repeat the same process for this library?

[wolfgangwalther]

Let's compare #4984, which merged a new external dependency (aeson-jsonpath) and was a smooth high-level review process for the most part.

Let's be realistic here: The end-result of that PR was just, that the dependency itself (aeson-jsonpath) was not reviewed at all. Or did you review its code carefully? I sure didn't. Taimoor wrote that library alone, without review. That probably works fairly well when developing against an existing spec.

You can easily achieve the same process in the monorepo: Just don't look at the code you intend to merge.

ISTM that maximizes ownership and pushes for higher quality, pushing to hackage would take longer, but it's not that merging #5139 is going to be much faster since there's still a good amount of code we haven't looked at.

Essentially you are asking for a way to not have to review this code and push responsibility for it elsewhere?

[mkleczek]

I'm still unsure if this is the right process. Let's compare #4984, which merged a new external dependency (aeson-jsonpath) and was a smooth high-level review process for the most part.
[...]
So my thinking is, why not repeat the same process for this library?

The same question can be asked about #5084 - I am sure there are good reasons why we decide to fork and maintain some libraries in-tree while use others as third party dependencies.
It would be good to list these criteria explicitly to avoid misunderstandings.

EDIT:

ISTM that maximizes ownership and pushes for higher quality

it is not that we don't have issues with third party libraries: warp, auto-update or fuzzyset required our work to address issues PostgREST is blamed for. And let's be honest: PostgREST is probably one of the biggest and most used consumer of these libraries.

EDIT2:
In general: well architected software is composed of independent and aiming for being generic and reusable pieces (components/libraries) glued together by a single "orchestrator" application component. Example of such generic/reusable pieces are #5139 but also Sieve cache implementation, which could have equally well be developed as separate project published on Hackage.
In case of PostgREST with its size and popularity it is more tempting to keep these independent pieces in-tree. The reason is that it becomes difficult to ensure high quality by outsourcing development to external projects that are very often understaffed and not motivated to keep up with PostgREST demands.

[steve-chavez]

Essentially you are asking for a way to not have to review this code and push responsibility for it elsewhere?

I was making the argument that we should just "trust" our core contributors wrt libraries, but Michal raised some great points above. Anyway, I was just bringing this option out but now it's settled.

[taimoorzaeem]

These tests should now be moved to test/io/test_zero_downtime.py.