Re: [PATCHES] odd output in restore mode

On Tue, 2008-07-22 at 17:19 -0700, Martin Zaun wrote:

> 1. Issues with applying the patch to CVS HEAD:

For me, the patch applies cleanly to CVS HEAD.

I do notice that there are two files "standby.sgml" and
"pgstandby.sgml". I can't see where "standby.sgml" comes from, but I
haven't created it; perhaps it is a relic of the SGML build process.
I've recreated my source tree since I wrote the patch also. Weird.

I'll redo the patch so it points at pgstandby.sgml, which is the one
thats listed as being in the main source tree.

> 2. Missing description for new command-line options in pgstandby.sgml
>
> - no description of the proposed new command-line options -h and -p?

These are done. The patch issues have missed those hunks.

> 3. No coding style issues seen
>
> Just one comment: the logic that selects the actual restore command to
> be used has moved from CustomizableInitialize() to main() -- a matter
> of personal taste, perhaps. But in my view the:
> + the #ifdef WIN32/HAVE_WORKING_LINK logic has become easier to read

Thanks

> 4. Issue: missing break in switch, silent override of '-l' argument?
>
> This behaviour has been in there before

Well spotted. I don't claim to test this for Windows.

> 5. Minor wording issue in usage message on new '-p' option
>
> I was wondering if the "always" in the usage text
> fprintf(stderr, " -p always uses GNU compatible 'cp' command on all platforms\n");
> is too strong, since multiple restore command options overwrite each
> other, e.g. "-p -c" applies Windows's "copy" instead of Gnu's "cp".

I was assuming you don't turn the switch off again immediately
afterwards.

> 6. Minor code comment suggestion
>
> Unrelated to this patch, I wonder if the code comments on all four
> time-related vars better read "seconds" instead of "amount of time":
> int sleeptime = 5; /* amount of time to sleep between file checks */
> int holdtime = 0; /* amount of time to wait once file appears full */
> int waittime = -1; /* how long we have been waiting, -1 no wait
> * yet */
> int maxwaittime = 0; /* how long are we prepared to wait for? */

As you say, unrelated to the patch.

> 7. Question: benefits of separate holdtime option from sleeptime?
>
> Simon Riggs wrote:
> > * provide "holdtime" delay, default 0 (on all platforms)
>
> Going back on the hackers+patches emails and parsing the code
> comments, I'm sorry if I missed that, but I'm not sure I've understood
> the exact tuning benefits that introducing the new holdtime option
> provides over using the existing sleeptime, as it's been the case
> (just on Win32 only).

This is central to the patch, since the complaint was about the delay
introduced by doing that previously.

> 8. Unresolved question of implementing now/later a "cp" replacement

The patch implements what's been agreed.

I'm not rewriting "cp", for reasons already discussed.

Not a comment to you Martin, but it's fairly clear that I'm not
maintaining this correctly for Windows. I've never claimed to have
tested this on Windows, and only included Windows related items as
requested by others. I need to make it clear that I'm not going to
maintain it at all, for Windows. If others wish to report Windows issues
then they can suggest appropriate fixes and test them also.

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support