WIP: Add new custom event loop for Linux I/O layer #456
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jesperpedersen
requested changes
Aug 14, 2024
decarv
force-pushed
the
new-linux-io-layer
branch
from
4cf0f12 to
24680d8
Compare
|
@decarv Rebase, squash, and CI |
decarv
force-pushed
the
new-linux-io-layer
branch
from
24680d8 to
45da25d
Compare
decarv
force-pushed
the
new-linux-io-layer
branch
from
45da25d to
87204f9
Compare
|
@decarv Please, look at the CI |
|
Please, see if you can get the liburing version check in place as well |
decarv
force-pushed
the
new-linux-io-layer
branch
2 times, most recently
from
f57a6f3 to
4a8a32c
Compare
fluca1978
requested changes
Sep 10, 2024
decarv
force-pushed
the
new-linux-io-layer
branch
from
4a8a32c to
4258ac0
Compare
Issue with shutdown ? |
Yes. I will investigate shutdown issues. However, from the CI output it could be anything, really. |
|
Try and a TRACE log file for gcc and clang, and see if there are pointers there... |
decarv
force-pushed
the
new-linux-io-layer
branch
2 times, most recently
from
e1b42f5 to
ceb9ec3
Compare
|
Please take a look at the latest commits. I decided to keep kqueue in this same PR for simplicity. I have just setup the testing environment for kqueue, so I haven't tested anything yet. |
jesperpedersen
requested changes
Sep 23, 2024
src/include/ev.h
Outdated
| { | ||
| enum ev_type type; /**< Event type. */ | ||
| int slot; /**< *CURRENTLY UNUSED* Slot number associated with the poll. */ | ||
| #if HAVE_URING |
|
Just a quick pass on the first part of the patch in order to give some comments that are probably general for the entire patch |
|
I still have issues with ipv6 listening addresses: pgagroal is unresponsive if |
I will investigate that further to understand what is happening. I don't see how ev.c is doing this (although it has to be it).
Thank you. Fixed. Already on the next commit. |
decarv
force-pushed
the
new-linux-io-layer
branch
from
ceb9ec3 to
d5d6680
Compare
It was inside an infinite loop waiting for all child processes to die. Maybe you can test that if you want. Please test the patch. I have added one or two |
|
CI failed because postgres crashed apparently. I will rebase and push again. |
decarv
force-pushed
the
new-linux-io-layer
branch
3 times, most recently
from
f6fb9bb to
d4de59b
Compare
|
@decarv What is up with |
Introduce ev.h and ev.c, establishing the foundation for the new custom event loop, `pgagroal_ev`. Replace previous dependencies on libev with the custom event loop. For Linux, implement support for io_uring with a fallback to epoll if io_uring is unavailable. For BSD, implement support for kqueue. Changelog ========= 2025-01-17: - Fix regression added to the Debug build. `waitpid()` inside `#if DEBUG` would block waiting for the running child processes. Add WNOHANG to the function options so the main process does not block. - Remove '\n' from `pagroal_log_*()` - Run uncrustify.sh - Remove `printf()` debugging - Modify CD/CI pipeline to improve function verify_running and enable the CD/CI for kqueue, which was disabled. - Change `pgagroal_log_error()` that informs errors in `kill()` to `pgagroal_log_debug()`. I suppose these errors should happen if pgagroal is not closely following up the death of its children. Therefore, there is no need to flood the log with error messages. Keep as debug because it is informative of something that could be improved. 2025-01-16: - Remove `pgagroal_ev_*_stop()` functions from main.c and vault.c as these functions are not intended to be called from a child process inheriting the ev loop. This does not make sense in io_uring and for epoll and kqueue, just closing the fds is enough as a precaution to prevent their use. - Enhance debugging steps in `sigchld_handler()` to help debug child processes termination. - Add error handling to `kill()` to help debug child processes termination. - Allow return error on `pgagroal_ev_io_stop()` instead of exit so the caller function can handle errors appropriately. - Add a call to `setpgid()` when worker is created to ensure child processes do not exit immediately if a SIGINT is issued in the controlling terminal. - Add a call to `pgagroal_ev_fork()` when a worker is created. 2025-01-13: - Fix ASan report in `pgagroal_ev_io_stop()`
decarv
force-pushed
the
new-linux-io-layer
branch
from
d4de59b to
2e31f0b
Compare
New compile flags I think... I will address this in a new PR shortly. |
|
As a side note, master could benefit from the CI script I'm currently running. |
|
Yep. I believe it was runtime errors being returned to the shell. TODO: store those logs instead of dumping in /dev/null. |
|
|
@decarv If you have patches that can be applied separately then create an issue and a pull request for it |
|
Testing commit 2e31f0b on Rocky 9.5. First run seems fine: but the second run fails: and on the EDIT: |
pgagroal cannot handle more connections than max connections, isn't this the reason? |
Uhm, |
|
I think that setting
So, Edit: I see this with both |
|
What does mean ? |
|
@fluca1978 I didn't pay attention to that. Sorry. Can you please try to reproduce again and show the steps? Maybe share the .conf and .databases? I cannot reproduce. That output happens when pgbench run is cancelled with Ctrl+C. That happens upstream also. @jesperpedersen Do you mean have "FD: -1" mid run when maxing out the connections? I don't see it happening here. Can you share more information? |
Just that one child exited with exit status 0. That message is printed by the main loop after receiving a SIGCHLD. |
No, I mean after the run, so if the run is short enough it means that the connections havn't timed out, which means that only |
|
@decarv It could be a related issue since if you force it with Anyway, test with |
So, it is more like TRACE ? Or is it important to keep at this level ? |
It s a trace. I will change it.
Sorry. I still do not understand. Luca's connections have all been used. I will do a run with |
|
@decarv I mean, when I do a similar run locally on my machine -- not Luca's actual run |
|
So you mean debug build, max_connections = 8, and limit file for the db I'm testing: 8 8 0. |
|
Yes, but We need 8 valid ones |
|
Here it is a run, built in debug mode, with databases configured as I don't see any And here follows the test with after a fresh start of both PostgreSQL and |
|
I took some time to test commit 2e31f0b on FreeBSD, with the same configuration as above ( When using a parallelism of but note the However, if I increase |
|
Thanks a lot @fluca1978. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Introduce ev.h and ev.c, establishing the
foundation for the new custom event loop,
pgagroal_ev.Replace previous dependencies on libev with the
custom event loop.
Implement support for io_uring with a fallback to
epoll if io_uring is unavailable.