Re: Proposing pg_hibernate - Mailing list pgsql-hackers

Hello,

I'm reviewing this patch.  I find this feature useful, so keep good work.

I've just begun the review of pg_hibernate.c, and finished reviewing other 
files.  pg_hibernate.c will probably take some time to review, so let me 
give you the result of my review so far.  I'm sorry for trivial comments.


(1)
The build on Windows with MSVC 2008 Express failed.  The error messages are 
as follows (sorry, they are in Japanese):


---------------------------------------- .\contrib\pg_hibernator\pg_hibernator.c(50): error C2373: 
'CreateDirectoryA' : 再定義されています。異なる型修飾子です。 .\contrib\pg_hibernator\pg_hibernator.c(196): error C2373: 
'CreateDirectoryA' : 再定義されています。異なる型修飾子です。 .\contrib\pg_hibernator\pg_hibernator.c(740): error C2198: 
'CreateDirectoryA' : 呼び出しに対する引数が少なすぎます。
----------------------------------------

The cause is that CreateDirectory() is an Win32 API.  When I renamed it, the 
build succeeded.  I think you don't need to make it a function, because its 
processing is simple and it is used only at one place.


(2)
Please look at the following messages.  Block Reader read all blocks 
successfully but exited with 1, while the Buffer Reader exited with 0.  I 
think the Block Reader also should exit 0 when it completes its job 
successfully, because the exit code 0 gives the impression of success.

LOG:  worker process: Buffer Saver (PID 2984) exited with exit code 0
...
LOG:  Block Reader 1: all blocks read successfully
LOG:  worker process: Block Reader 2 (PID 6064) exited with exit code 1

In addition, the names "Buffer Saver" and "Block Reader" don't correspond, 
while they both operate on the same objects.  I suggest using the word 
Buffer or Block for both processes.


(3)
Please add the definition of variable PGFILEDESC in Makefile.  See 
pg_upgrade_support's Makefile for an example.  It is necessary for the 
versioning info of DLLs on Windows.  Currently, other contrib libraries lack 
the versioning info.  Michael Paquier is adding the missing versioning info 
to other modules for 9.5.


(4)
Remove the following #ifdef, because you are attempting to include this 
module in 9.5.

#if PG_VERSION_NUM >= 90400


(5)
Add header comments at the beginning of source files like other files.


(6)
Add user documentation SGML file in doc/src/sgml instead of README.md.
For reference, I noticed the following mistakes in README.md:

instalation -> installation
`Block Reader` threads -> `Block Reader` processes


(7)
The message

"could not open \"%s\": %m"

should better be:

"could not open file \"%s\": %m"

because this message is already used in many places.  Please find and use 
existing messages for other places as much as possible.  That will reduce 
the translation efforts for other languages.


(8)
fileClose() never returns false despite its comment:

/* Returns true on success, doesn't return on error */


(9)
I think it would be better for the Block Reader to include the name and/or 
OID of the database in its success message:

LOG:  Block Reader 1: restored 14 blocks


(10)
I think the save file name should better be <database OID>.save instead of 
<some arbitrary integer>.save.  That also means %u.save instead of %d.save. 
What do you think?


(11)
Why don't you remove misc.c, removing unnecessary functions and keeping just 
really useful ones in pg_hibernator.c?  For example, I don't think 
fileOpen(), fileClose(), fileRead() and fileWrite() need not be separate 
functions (see src/backend/postmaster/pgstat.c).  And, there's only one call 
site of the following functions:

readDBName
getSavefileNameBeingRestored
markSavefileBeingRestored

Regards
MauMau