Re: Proposing pg_hibernate

Hello,

I've finished reviewing the code.  I already marked this patch as waiting on 
author.  I'll be waiting for the revised patch, then proceed to running the 
program when the patch seems reasonable.

(12)
Like worker_spi, save and restore errno in signal handlers.


(13)
Remove the following comment, and similar ones if any, because this module 
is to be included in 9.5 and above.
/* * In Postgres version 9.4 and above, we use the dynamic background worker * infrastructure for BlockReaders, and the
BufferSaverprocess does the * legwork of registering the BlockReader workers. */
 


(14)
Expand the body following functions at their call sites and remove the 
function definition, because they are called only once.  It would be 
straightforward to see the processing where it should be.

* DefineGUCs
* CreateDirectory


(15)
I don't understand the use case of pg_hibernator.enabled.  Is this really 
necessary?  In what situations does this parameter "really" matter?  If it's 
not crucial, why don't we remove it and make the specification simpler?


(16)
As mentioned above, you can remove CreateDirectory().  Instead, you can just 
mkdir() unconditionally and check the result, without first stat()ing.


(17)
Use AllocateDir/ReadDir/FreeDir instead of opendir/readdir/closedir in the 
server process.  Plus, follow the error handling style of other source files 
using AllocateDir.


(18)
The local variable hibernate_dir appears to be unnecessary because it always 
holds SAVE_LOCATION as its value.  If so, remove it.


(19)
I think the severity below should better be WARNING, but I don't insist.

ereport(LOG, (errmsg("registration of background worker failed")));


(20)
"iff" should be "if".
/* Remove the element from pending list iff we could register a worker 
successfully. */



(21)
How is this true?  Does the shared system catalog always have at least one 
shared buffer?
  /* Make sure the first buffer we save belongs to global object. */  Assert(buf->database == InvalidOid);
...   * Special case for global objects. The sort brings them to the   * front of the list.


(22)
The format should be %u, not %d.
 ereport(log_level,   (errmsg("writer: writing block db %d filenode %d forknum %d blocknum 
%d",     database_counter, prev_filenode, prev_forknum, buf->blocknum)));


(23)
Why is the first while loop in BlockReaderMain() necessary?  Just opening 
the target save file isn't enough?



(24)
Use MemoryContextAllocHuge().  palloc() can only allocate chunks up to 1GB.
 * This is not a concern as of now, so deferred; there's at least one other * place that allocates (NBuffers *
(much_bigger_struct)),so this seems to * be an acceptable practice. */
 
saved_buffers = (SavedBuffer *) palloc(sizeof(SavedBuffer) * NBuffers);


(25)
It's better for the .save files to be created per tablespace, not per 
database.  Tablespaces are more likely to be allocated on different storage 
devices for I/O distribution and capacity.  So, it would be more natural to 
expect that we can restore buffers more quickly by letting multiple Block 
Readers do parallel I/O on different storage devices.


(26)
Reading the below description in the documentation, it seems that Block 
Readers can exit with 0 upon successful completion, because bgw_restart_time 
is set to BGW_NEVER_RESTART.

"If bgw_restart_time for a background worker is configured as 
BGW_NEVER_RESTART, or if it exits with an exit code of 0 or is terminated by 
TerminateBackgroundWorker, it will be automatically unregistered by the 
postmaster on exit."


(27)
As others said, I also think that Buffer Saver should first write to a temp 
file, say *.tmp, then rename it to *.save upon completion.  That prevents 
the Block Reader from reading possibly corrupted half-baked file that does 
not represent any hibernated state.


Regards
MauMau