nixos/postgresql: add ensureDatabases.*.withOptions

[luochen1990]

Description of changes

Make services.postgresql.ensureDatabases support not only str as dbname, but also submodule to specify withOptions like OWNER, TEMPLATE, ENCODING, LOCALE, etc.

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
  • sandbox = relaxed
  • sandbox = true
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
• Fits CONTRIBUTING.md.

---

Add a 👍 reaction to pull requests you find important.

[drupol]

Nice changes, diff LGTM at first sight. I haven't tested it.

[ambroisie]

Could the option use apply instead to convert stringly-typed options to the module, with an empty withOptions?

This would make the module implementation simpler.

[luochen1990]

Could the option use apply instead

I don't quite understand the meaning of this sentence, possibly because I am not a native English speaker. Do you mean assigning a separate type to each option rather than using str for all of them?

[ambroisie]

@luochen1990 apply is a function that can be used to apply arbitrary modifications to a (merged) option.

In this instance, I'm suggesting using it to convert from the stringly-typed option to the full-featured module, which would mean not having to worry about handling bare strings in the rest of the module.

EDIT: I'm having trouble finding the documentation for this in the manual, I could have sworn it was explained somewhere in the NixOS manual...

[luochen1990]

apply is a function that can be used to apply arbitrary modifications to a (merged) option

Good suggestion! thanks for telling me this, I will change to use that now!

[drupol]

Can you please adjust the PR title and commit log message... and make sure there's only 1 commit in here?

[luochen1990]

Can you please adjust the PR title and commit log message... and make sure there's only 1 commit in here?

What should the PR title look like?

[drupol]

Can you please adjust the PR title and commit log message... and make sure there's only 1 commit in here?

What should the PR title look like?

Please read again: https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md#commit-conventions

[luochen1990]

make sure there's only 1 commit

Because GitLab supports selecting the squash option when merging, I originally thought GitHub also supported this feature, so I would usually try to keep the commit history as much as possible, but I didn't expect GitHub to have fewer features than GitLab ..

[drupol]

make sure there's only 1 commit

Because GitLab supports selecting the squash option when merging, I originally thought GitHub also supported this feature, so I would usually try to keep the commit history as much as possible, but I didn't expect GitHub to have fewer features than GitLab ..

There's no consensus on this in nixpkgs yet, but Github does support it partially. IMHO, it's up to the contributor to make sure its commits are properly organised.

[luochen1990]

Can you please adjust the PR title and commit log message... and make sure there's only 1 commit in here?

I think this is already solved, but I don't know how to mark it as solved.

[drupol]

Yes, I received all the notifications and saw it. The Diff LGTM but I'll let someone else handle the review carefully.

[luochen1990]

@drupol There might be string literal escaping issue, I'm not sure if there are existing codes to reuse.

[drupol]

I won't be able to test it, I'm not using postgresql. That's the reason why I left this PR for someone else.

[Ma27]

AFAIU the options part is a tombstone as soon as the DB is created, correct? I.e. if the DB exists and you change one of the options, nothing happens.

Doesn't seem very intuitive (or declarative!) to me.

I think the entire approach here is inherently flawed. Also, the reminder that the ensure* stuff caused major pain during the last two NixOS releases for the postgresql maintainers: #318777, #266270.

\I'd rather not merge.

[luochen1990]

Thanks for your reply. I have give my opinion about "the ensure anti-pattern"* in #107342 (comment)

I think we can improve it instead of just delete it and use a worse solution to just make less promise to user.

[Ma27]

For the current state of the change, all of my criticism voiced above still applies, hence "requesting changes" for now.

Will get back to you in the other thread this afternoon (CEST), OK? :)