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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers