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.

Sign up for GitHub

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

Jump to bottom

pgpool: 3.4.14 -> 4.0.5 #66224

Merged
merged 1 commit into from Aug 7, 2019
Merged

pgpool: 3.4.14 -> 4.0.5 #66224

merged 1 commit into from Aug 7, 2019

Conversation

takeda
Copy link
Contributor

@takeda takeda commented Aug 6, 2019

Motivation for this change

I needed to use latest version of pgpool (4.0.5) and wanted to contribute my change back.
This change also patches the code to allow providing an absolute path for pool_passwd. Originally pgpool is looking for the pool_passwd in the same directory where configuration is located. Having it accept an absolute path gives more flexibility.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS (pgbouncer doesn't compile on OS X without an extra effort so I didn't bother)
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests) (no tests that I'm aware of)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-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) (was getting an error about stdenv: error: cannot auto-call a function that has an argument without a default value ('stdenv'))
  • Ensured that relevant documentation is up to date (this is just a package and I don't believe there's a documentation beyond the meta data)
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @ (no idea who is a maintainer for it)

@danbst
Copy link
Contributor

danbst commented Aug 6, 2019

As for patch itself - wouldn't it be more safe to use absolute path only if pool_config->pool_passwd starts with /, and resolve relative in other cases?

In general, we avoid patches which change logic too much, and this change requires package documentation changes. This restriction can be lifted if patch is submitted upstream.

@takeda
Copy link
Contributor Author

takeda commented Aug 6, 2019

@danbst that's a good suggestion, will try that

@takeda
pgpool: 3.4.14 -> 4.0.5
f38f605 
Updated to PGPool-II 4.0.5 and modify the code to allow absolute path
for pool_passwd file.
@takeda
Copy link
Contributor Author

takeda commented Aug 6, 2019

@danbst Looks like the patch around watchdog is indeed no longer needed so I removed it (I must have old version of nixpkgs) I also applied your suggestion to treat option as an absolute path if it starts with a slash.

@danbst
Copy link
Contributor

danbst commented Aug 7, 2019

@takeda I've proposed patch upstream http://www.sraoss.jp/pipermail/pgpool-hackers/2019-August/003366.html. Let's wait a bit for feedback, but overall I think this is fine and can be merged.

@danbst danbst merged commit b1d00b7 into NixOS:master Aug 7, 2019
@danbst
Copy link
Contributor

danbst commented Aug 7, 2019

@takeda thanks for your contribution! The patch would most likely be included upstream, so I went ahead and merged it.

@takeda takeda deleted the pgpool branch August 7, 2019 15:37
@takeda
Copy link
Contributor Author

takeda commented Aug 7, 2019

@danbst thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danbst danbst danbst left review comments

Assignees
No one assigned
Labels
10.rebuild-darwin: 0 10.rebuild-linux: 1-10 12. first-time contribution
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@takeda @danbst @Janik-Haag