Re: pg_rewind in contrib

Here is a random bag of comments for the v5 patch:

pg_xlogdump fails to build:
 CC   xlogreader.o CC   rmgrdesc.o
../../src/include/access/rmgrlist.h:32:46: error: 'dbase_desc' undeclared here (not in a function)PG_RMGR(RM_DBASE_ID,
"Database",dbase_redo, dbase_desc, dbase_identify, NULL, NULL)                                             ^
 
rmgrdesc.c:33:10: note: in definition of macro 'PG_RMGR' { name, desc, identify},         ^
../../src/include/access/rmgrlist.h:32:58: error: 'dbase_identify' undeclared here (not in a
function)PG_RMGR(RM_DBASE_ID,"Database", dbase_redo, dbase_desc, dbase_identify, NULL, NULL)
                            ^
 
rmgrdesc.c:33:16: note: in definition of macro 'PG_RMGR' { name, desc, identify},               ^
../../src/Makefile.global:732: recipe for target 'rmgrdesc.o' failed
make[2]: *** [rmgrdesc.o] Error 1


SGML files should use 1-space indentation, not 2.

In high-availability.sgml, pg_rewind could be a link.

In ref/pg_rewind.sgml,

-P option listed twice.

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?

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.


src/bin/pg_rewind/.gitignore lists files for pg_regress use, which is not used anymore.

src/bin/pg_rewind/Makefile says "Makefile for src/bin/pg_basebackup".

There should be an installcheck target.

RewindTest.pm should be in the t/ directory.

Code like this:

+       if (map->bitmap == NULL)
+           map->bitmap = pg_malloc(newsize);
+       else
+           map->bitmap = pg_realloc(map->bitmap, newsize);

is unnecessary.  You can just write
   map->bitmap = pg_realloc(map->bitmap, newsize);

because realloc handles NULL.

Instead of FILE_TYPE_DIRECTORY etc., why not use S_IFDIR etc.?

About this code

+           if (!exists)
+               action = FILE_ACTION_CREATE;
+           else
+               action = FILE_ACTION_NONE;

action is earlier initialized as FILE_ACTION_NONE, so the second
branch is redundant.  Alternatively, remove the earlier
initialization, so that maybe the compiler can detect if action is not
assigned in some paths.

Error messages should not end with a period.

Some calls to pg_fatal() don't have the string end with a newline.

In libpqProcessFileList(), I would tend to put the comment outside of
the SQL command string.

Mkvcbuild.pm changes still refer to pg_rewind in contrib.



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?

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.