Differential D30330
rc.conf(5): Add _limits, _login_class, and _oomprotect Authored by debdrup on May 18 2021, 12:52 PM. Tags None Referenced Files
Details Add a few very useful variables that might easily be overlooked, since At least _oomprotect has existed since 11.0-RELEASE, and doesn't appear While here, also add a reference to rc.subr(8). Tested using mantra, igor, and mandoc.
Diff Detail
Event TimelineComment Actions I found out the hard way, that an rc script overriding start_cmd ignores some mechanisms. This may be by design. Those ignored are at the least:
Unfortunately, some ports like PostgreSQL redefine start_cmd which would make _oomprotect="ALL" silently not work for the database. I am not sure where it would be a good place, but I think it would be worthwhile to document that redefining ${name}_cmd has such a pitfall.
Comment Actions It would also be nice to have discoverability from protect(1). Something to the notion of: "Daemons can be automatically protected using _oomprotect in rc.conf(5)" and a SEE ALSO to rc.conf(5). Comment Actions Yeah, documenting this is a good idea - but I feel like it's outside the scope of this review. Absolutely, but it also belongs in its own review. ;) Comment Actions One last nit, sorry - could you sort these alphabetically, with _limits and _login_class above _nice and _oomprotect after it? Comment Actions I was thinking about that - but I'll want to sort all of the (name)_ variables alphabetically in that case. Comment Actions In that particular case, that may not actually be a bad thing, although for the wrong reason. Protecting processes with large or unpredictable memory use is likely to deadlock the system (or close to it) if it runs out of memory, while killing a database server is bad but is likely to be picked up properly by monitoring systems. Comment Actions Killing PostgreSQL processes leads to the database entering recovery mode (as it properly assumes that corruption may have happened). This is a lenghty process for large databases. If someone went as far as to add postgresql_oomprotect="ALL" to his rc.conf then that person explicitly wanted the database protected and likely weighted the potential deadlock. On our production systems the cases where I found PostgreSQL dead were almost always caused by some other process (not Postgres) going crazy. In our case, we have a hardware watchdog and can live with the machine rebooting itself in the unlikely case that it was actually PostgreSQL deadlocking the machine but reducing the amount of cases where the database has been restarted with no good reason is a much better tradeoff for us. We are not the only people assuming that you can do postgresql_oomprotect="ALL" and expect it to behave. Someone pointed me at this article: https://fluca1978.github.io/2021/04/02/OOMKillerFreeBSD.html which tried the same solution and also learned that it is not working with the provided rc script. Comment Actions Try to address feedback by a.wolk I'm not completely sure about the use of Bd and Ed macros, but it's the
Comment Actions This landed with commit bd6dce978c1a, but I forgot to add the differential revision tag. To make up for it, I responded to the dev-commits-src-main@ and dev-commits-src-all@. | |||||||||||||||||||||||||||||||||||||||||||||||||||||