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/postgresql-wal-receiver: add module #63799
Conversation
Note: I'm not sure whether the services defined in this module should use |
1221a0e
to
69235db
Compare
I have a few nitpicks for module itself, but biggest concern is that test is missing. Can you provide a test file for this? |
507962d
to
4a2520e
Compare
Test added. |
in mkIf (receivers != { }) { | ||
users = { | ||
groups.postgresql-wal-receiver = { }; | ||
users.postgresql-wal-receiver = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
StackOverflow says better stick to short usernames. Something like walreceiver
would be fine, what do you think?
And also, if it is possible to implement master-slave replication using this module, maybe the running user should be configurable? In test you explicitly do chown -R postgres:postgres
, but it may not strictly required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
StackOverflow says better stick to short usernames. Something like walreceiver would be fine, what do you think?
Indeed. I've modified the user and group accordingly. The user's description should be enough.
And also, if it is possible to implement master-slave replication using this module, maybe the running user should be configurable?
This program and module are solely for proper WAL archiving.
Making the user and group configurable might be useful but I cannot think of a real use-case.
In test you explicitly do chown -R postgres:postgres, but it may not strictly required.
Indeed, but on a real setup, the WAL receiver would run on a separate machine and the restoration would have used a copy of the log.
Niiice! Looking forward on future module for master-slave setup using NixOS! Most of my comments are stylistic, and I'm fine with merging this as-is if you can demonstrate it works with all PGs. |
1284a5d
to
ee4ef75
Compare
@danbst wrote:
Thanks for your review. I've modified the test to run it against all the available PostgreSQL package versions. |
ee4ef75
to
e50d8f8
Compare
e50d8f8
to
2fe1a67
Compare
Still, pg 9.4 doesn't pass test. 9.5, 9.6, 10 and 11 work. If it is complicated to fix test for pg9.4, then maybe we should skip it. |
@danbst wrote:
I just fixed an issue with the absence of partial segments. It should work now. |
@pacien oh, there is one more issue. You create a new user https://github.com/NixOS/nixpkgs/blob/release-19.03/nixos/modules/misc/ids.nix |
@danbst wrote:
That file says "don't use uids above 399!" and we're dangerously approaching this limit. |
@pacien yeah, it would be the best |
Is it actually possible to define the same user in multiple modules? |
yes:
the idea is that if we assign same option same value, it would be merged correctly. It won't work only for |
It seems like the UID and GID attributes aren't merged when both modules are enabled:
|
@pacien oh shit.
merge function should really be |
seems it is the simplest solution. We can also introduce |
@danbst See #60732 (comment) (and later comments) for the running out of uid/gid thing |
2fe1a67
to
9a6f8ae
Compare
Rebased and amended. |
testScript = '' | ||
# make an initial base backup | ||
$machine->waitForUnit('postgresql'); | ||
$machine->waitForUnit('postgresql-wal-receiver-main'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pg 9.4 still didn't work. I figured out, that it was because walreceiver polls PG every 5 seconds, and it happens that stop postgresql
is done during this period "between healthchecks". Timing issue, not sure why it is fine on 9.5+
I see two solutions:
- add
+ # WAL receiver healthchecks PG every 5 seconds, so let's be sure they have connected each other
+ # required only for 9.4
+ $machine->sleep(5);
after this line
- or wait until some file occurs in WAL backup dir before stopping PG
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added the suggested delay and amended.
I couldn't reproduce the timing issue with pg 9.4 on my machine in the first place.
Could you confirm that this solves the issue on yours?
9a6f8ae
to
e268038
Compare
Motivation for this change
This PR adds a service module for
pg_receivewal
, a program which streams the transaction log from a PostgreSQL cluster into a local directory for PITR.To do
nixos testusepostgres
user (blocked by nixos/postgresql-wal-receiver: add module #63799 (comment), lib/types: change merge strategy forstr
,int
,float
,path
andenum
#65380)Things done
sandbox
innix.conf
on non-NixOS)nix-shell -p nix-review --run "nix-review wip"
./result/bin/
)nix path-info -S
before and after)