Re: pg_rewind in contrib

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