Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nixos/postgres Add syslog option #94231

Closed
wants to merge 2 commits into from
Closed

nixos/postgres Add syslog option #94231

wants to merge 2 commits into from

Conversation

@jappeace
Copy link
Contributor

jappeace commented Jul 30, 2020

Motivation for this change

Having the option to use postgres to log all your queries should
be quite valuable to many users.

Doing this at application level is usually less reliable and slower.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Having the option to use postgres to log all your queries should
be quite valuable to many users.

Doing this at application level is usually less reliable and slower.
@aanderse
Copy link
Contributor

aanderse commented Jul 31, 2020

Does logging work as desired if you add this new configuration to extraConfig instead of the changes you made here?

@aanderse
Copy link
Contributor

aanderse commented Aug 1, 2020

Thanks for answering. Two things:

  • I don't think you need to change log_destination, but I'm not sure.
  • Isn't the name enableSystemLogging misleading? All logging is too the system log (journald) already. I would also think that people might want to only turn a few of these options on instead of all sometimes. Maybe an attribute set representing the configuration file would make the situation better here.
@jappeace
Copy link
Contributor Author

jappeace commented Aug 1, 2020

I don't think you need to change log_destination, but I'm not sure.

Default behavior appears to be logging to some file in /var/lib/postgresql/PG_VERSION/log. Enabling syslog will put everything into journalctl on a systemd machine. postgres predates system wide logging schemes so it make sense the default would be a file.

I would also think that people might want to only turn a few of these options on instead of all sometimes

Probably yeah. Especially log_statement all may not always be desirable for larger apps.

Maybe an attribute set representing the configuration file would make the situation better here.

So you mean a configuration option per postgres option nested within something called logging? Yeah I can rework it into that.

@jappeace
Copy link
Contributor Author

jappeace commented Aug 1, 2020

Actually I think that writing of files into /var/lib/postgresql/PG_VERSION/log is because of the logging collector. I'm not sure what happens to stderr.

@aanderse
Copy link
Contributor

aanderse commented Aug 1, 2020

Actually I think that writing of files into /var/lib/postgresql/PG_VERSION/log is because of the logging collector. I'm not sure what happens to stderr.

I believe stderr is be redirected to journald.

@aanderse
Copy link
Contributor

aanderse commented Aug 1, 2020

So you mean a configuration option per postgres option nested within something called logging? Yeah I can rework it into that.

What about something like this?

@jappeace
Copy link
Contributor Author

jappeace commented Aug 1, 2020

Maybe that didn't work because I still had the log collector enabled? Wait I'll try again

@jappeace
Copy link
Contributor Author

jappeace commented Aug 1, 2020

Yeah it was that, so enabling the logging collector will capture the stderr, and then if you enable syslog you get both files and syslog output. The advantage being that logging collector will never drop a message (at a small risk of freezing the system if it's unable to write for some reason).

@aanderse
Copy link
Contributor

aanderse commented Aug 1, 2020

@jappeace what about something like this?

@jappeace
Copy link
Contributor Author

jappeace commented Aug 1, 2020

@aanderse I just saw yeah, but I didn't want to break backward compatibility, and I also like having the types, there are several fields which are simply weird booleans or enums. But you're right, it's pretty much a one to one mapping.

@aanderse
Copy link
Contributor

aanderse commented Aug 1, 2020

@aanderse I just saw yeah, but I didn't want to break backward compatibility, and I also like having the types, there are several fields which are simply weird booleans or enums. But you're right, it's pretty much a one to one mapping.

The problem with applying type checking to less commonly used configuration values (ie. making them options) is that you end up replicating the entire configuration file eventually. See NixOS/rfcs#42 for an interesting discussion on the pros and cons of options vs a single settings option (for less common configuration). This RFC is likely to become the recommendation to module authors going forward.

@jappeace
Copy link
Contributor Author

jappeace commented Aug 1, 2020

@aanderse Okay, I see, so module size appears to be a big problem (I thought it was the opposite, more = better). These logging options then shouldn't be added as separate options but as in your suggestion. You should make a PR for that.

@jappeace jappeace closed this Aug 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.