On 01/16/2015 03:30 AM, Peter Eisentraut wrote:
Here is a random bag of comments for the v5 patch:
Addressed most of your comments, and Michael's. Another version attached. More on a few of the comments below.
The option --source-server had be confused at first, because the entry above under --source-pgdata also talks about a "source server". Maybe --source-connection would be clearer?
Hmm. I would rather emphasize that the source is a running server, than the connection to the server. I can see the confusion, though. What about phrasing it as:
--source-pgdata Specifies path to the data directory of the source server, to synchronize the target with. When <option>--source-pgdata</> is used, the source server must be cleanly shut down.The point is that whether you use --source-pgdata or --source-server, the source is a PostgreSQL server. Perhaps it should be mentioned here that you only need one of --source-pgdata and --source-server, even though the synopsis says that too.
Another idea is to rename --source-server to just --source. That would make sense if we assume that it's more common to connect to a live server:
pg_rewind --target mypgdata --source "host=otherhost user=dba" pg_rewind --target mypgdata --source-pgdata /other/pgdata
Reference pages have standardized top-level headers, so "Theory of operation" should be under something like "Notes". Similarly for "Restrictions", but that seems important enough to go into the description.
Oh. That's rather limiting. I'll rename the "Restrictions" to "Notes" - that seems to be where we have listed limitations like that in many other pages. I also moved Theory of operation as a "How it works" subsection under "Notes".
The top-level headers aren't totally standardized in man pages. Googling around, a few seem to have a section called "IMPLEMENTATION NOTES", which would be a good fit here. We don't have such sections currently, but how about it?
There should be an installcheck target.
Doesn't seem appropriate, as there are no regression tests that would run against an existing cluster. Or should it just use the installed pg_rewind and postgres binary, but create the temporary clusters all the same?
RewindTest.pm should be in the t/ directory.
I used a similar layout in src/test/ssl, so that ought to be changed too if we're not happy with it.
"make check" will not find the module if I just move it to t/, so that would require changes to Makefiles or something. I don't know how to do that offhand.
Instead of FILE_TYPE_DIRECTORY etc., why not use S_IFDIR etc.?
I don't see what that would buy us. Most of the code only knows about a few S_IF* types, namely regular files, directories and symlinks. Those are the types that there are FILE_TYPE_* codes for. If we started using the more general S_IF* constants, it would not be clear which types can appear in which parts of the code. Also, the compiler would no longer warn about omitting one of the types in a "switch(file->type)" statement.
TestLib.pm addition command_is sounds a bit wrong. It's evidently modelled after command_like, but that now sounds wrong too. How about command_stdout_is?
Works for me. How about also renaming command_like to command_stdout_like, and committing that along with the new command_stdout_is function as a separate patch, before pg_rewind?
The test suite needs to silence all non-TAP output. So psql needs to be run with -q pg_ctl with -s etc. Any important output needs to be through diag() or note(). Test cases like ok 6 - psql -A -t --no-psqlrc -c port=10072 -c SELECT datname FROM pg_database exit code 0 should probably get a real name. The whole structure of the test suite still looks too much like the old hack. I'll try to think of other ways to structure it.
Yeah. It currently works with callback functions, like:
test program:
-> call RewindTest::run_rewind_test
set up both cluster
-> call before_standby callback
start standby
-> call standby_following_master callback
promote standby
-> call after_promotion callback
stop master
run pg_rewind
restart master
-> call after_rewind callback
It might be better to turn the control around, so that all the code
that's now in the callbacks are in the test program's main flow, and
test program calls functions in RewindTest.sh to execute the parts that
are currently between the callbacks:
test program -> call RewindTest::setup_cluster do stuff that's now in before_standby callback -> call RewindTest::start_standby do stuff that's now in standby_following_master callback -> call RewindTest::promote_standby do stuff that's now in after_promotion callback -> call RewindTest::run_pg_rewind do stuff that's now in after_rewind callback - Heikki
pg_rewind-bin-6.patch.gz
Description: application/gzip
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
