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.

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.

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.

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.

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

#if PG_VERSION_NUM >= 90400

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

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

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.

fileClose() never returns false despite its comment:

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

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

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?

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:



Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to