Thank you!

The RFC this PR follows: NixOS/rfcs#42
The idea is to only support essential options an upstream program provides, the other options can be set in an easy to use attribute set.

The idea is to only support essential options an upstream program provides, the other options can be set in an easy to use attribute set.

In this scenario the motivation is to replace extraConfig which can't be properly merged with settings which can be properly merged, not replace any existing options or imply no new options should be added.

@aanderse I was just summarizing how I understood that RFC, not what this PR does. My apologies for the confusion.

Feedback addressed.

@GrahamcOfBorg test postgresql postgresql-wal-receiver

Merge conflict resolved. Anyone keen on merging?

It would be kinder to users to have a smooth upgrade path.
Is it possible to deprecate extraConfig for one release then remove it in the next release?

It would be kinder to users to have a smooth upgrade path.
Is it possible to deprecate extraConfig for one release then remove it in the next release?

While I hate making changes that instantly force users to adapt without prior notice, in the case of extraConfig it is "worth it", at least in my opinion.

When asked about keeping both a settings and extraConfig option @infinisil had this to say, which I think is quite relevant:

I'd rather not if possible, because then we have problem of not being able to merge values set in extraConfig with all other values, there are essentially two incompatible ways of specifying configuration then.

Further to this I would argue that migrating from extraConfig to settings is very trivial so it shouldn't take more than a minute or two.

Have I sufficiently addressed your concerns? I'm happy to continue the discussion and consider this further if not.

I suspect that users with existing nixos configurations may resent this as a gratuitous change providing no benefit to them, especially if they need to modify multiple services, or if option merging semantics change. Just my 2c.

Related to this PR itself, I have

services.postgresql.extraConfig = ''
  session_preload_libraries = 'auto_explain'
  auto_explain.log_min_duration = '5s'
'';

Should the postgesql settings option support nested attrsets? E.g.

services.postgresql.settings.auto_explain.log_min_duration = "5s";

I suspect that users with existing nixos configurations may resent this as a gratuitous change providing no benefit to them, especially if they need to modify multiple services, or if option merging semantics change. Just my 2c.

I would highly encourage you to express your opinions on this in the NixOS/rfcs#42 thread. The more feedback an RFC gets (constructive criticism included) the better the end result.

Should the postgesql settings option support nested attrsets? E.g.

I'm not much of a postgresql user. I was unaware that such options existed. Documentation should be updated to reflect this and explain values like that should be quoted, like so:

services.postgresql.settings."auto_explain.log_min_duration" = "5s";

As a postgresql user would you have any motivation to contribute to this module by enhancing the documentation?

I just posted another comment about backwards compatibility in the RFC (saying it's up to the module writer to decide how to handle backwards compatibility). I guess the original comment by me that @aanderse linked is still relevant, but I think I mostly wrote that in relation to new NixOS modules. So if you create a new module, it should definitely not have an extraConfig if it can instead be settings. The RFC was mainly just intended for new modules because I knew backwards compatibility could be a problem.

While I'd love for all existing modules to eventually adapt the settings approach, I think backwards compatibility is still important, and for popular modules like postgresql it might make sense to keep supporting extraConfig for a while before deprecating it, even though settings won't have its full potential during that time. Maybe one release of it being supported with a deprecation warning would be nice.

@rvl NixOS is a pretty bleeding edge distro. Unfortunately inconvenient changes happen. I really like the welcoming and collaborative nature of the NixOS community so when I ask: "is this migration really that big of a hassle?" I'm just trying to better understand your perspective.

Here is my perspective: I manage upwards of 40 NixOS machines, each of which has a number of changes required to migrate to 20.09. Out of all the changes required to migrate to 20.09 I personally see extraConfig to settings as one of the least impacting and quickest to manage.

So now that we have had this discussion what can you do if you're still not satisfied with the situation? I would be disappointed if you made a PR add extraConfig back, but I wouldn't discourage it. If you see this as a breaking which isn't acceptable then we can quickly talk to the module maintainers and release managers to get your PR into 20.09. I hope you don't feel this way still, but I'll respect your opinion and try to support your efforts the best I can.

I think I have already made my perspective clear, thanks.
Please make sure you put something in the 20.09 release notes about this.