Re: [PATCHES] odd output in restore mode

Below my comments on the CommitFest patch:
pg_standby minor changes for Windows

Simon, I'm sorry you got me, a Postgres newbie, signed up for
reviewing your patch ;)

To start with, I'm not quite sure of the status of this patch
since Bruce's last comment on the -patches alias:

Bruce Momjian wrote:
> OK, based on these observations I think we need to learn more about the
> issues before making any changes to our code.

From easy to difficult:

1. Issues with applying the patch to CVS HEAD:

The second file in the patch
Index: doc/src/sgml/standby.sgml
appears to be misnamed -- the existing file in HEAD is
Index: doc/src/sgml/pgstandby.sgml

However, still had issues after fixing the file name:

md(at)Garu:~/pg/pgsql$ patch -c -p0 < ../pg_standby.patch
patching file contrib/pg_standby/pg_standby.c
patching file doc/src/sgml/pgstandby.sgml
Hunk #1 FAILED at 136.
Hunk #2 FAILED at 168.
Hunk #3 FAILED at 245.
Hunk #4 FAILED at 255.
4 out of 4 hunks FAILED -- saving rejects to file doc/src/sgml/pgstandby.sgml.rej

2. Missing description for new command-line options in pgstandby.sgml

Simon Riggs wrote:
> Patch implements
> * recommendation to use GnuWin32 cp on Windows

Saw that in the changes to pgstandby.sgml, and looks ok to me, but:
- no description of the proposed new command-line options -h and -p?

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

4. Issue: missing break in switch, silent override of '-l' argument?

This behaviour has been in there before and is not addresses by the
patch: The user-selected Win32 "mklink" command mode is never applied
due to a missing 'break' in CustomizableInitialize():

switch (restoreCommandType)
{
case RESTORE_COMMAND_WIN32_MKLINK:
SET_RESTORE_COMMAND("mklink", WALFilePath, xlogFilePath);
case RESTORE_COMMAND_WIN32_COPY:
SET_RESTORE_COMMAND("copy", WALFilePath, xlogFilePath);
break;

A similar behaviour on Non-Win32 platforms where the user-selected
"ln" may be silently changed to "cp" in main():

#if HAVE_WORKING_LINK
restoreCommandType = RESTORE_COMMAND_LN;
#else
restoreCommandType = RESTORE_COMMAND_CP;
#endif

If both Win32/Non-Win32 cases reflect the intended behaviour:
- I'd prefer a code comment in the above case-fall-through,
- suggest a message to the user about the ignored "ln" / "mklink",
- observe that the logic to override of the '-l' option is now in two
places: CustomizableInitialize() and main().

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".

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? */

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).

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

Simon Riggs wrote:
> On Tue, 2008-07-01 at 13:44 +0300, Heikki Linnakangas wrote:
>> This seems pretty kludgey to me. I wouldn't want to install GnuWin32
>> utilities on a production system just for the "cp" command, and I don't
>> know how I would tune holdtime properly for using "copy". And it seems
>> risky to have defaults that are known to not work reliably.
>>
>> How about implementing a replacement function for "cp" ourselves? It
>> seems pretty trivial to do. We could use that on Unixes as well, which
>> would keep the differences between Win32 and other platforms smaller,
>> and thus ensure the codepath gets more testing.
>
> If you've heard complaints about any of this from users, I haven't.
> AFAIK we're doing this because it *might* cause a problem. Bear in mind
> that link is the preferred performance option, not copy. So AFAICS we're
> tuning a secondary option on one specific port, without it being a
> raised issue and in an area of code that will be superceded in the next
> release.
>
> So further embellishments would be a long way down my own priority list,
> putting it politely. Yet I have no objections to the suggestion overall;
> we have done that already for alter tablespace.

Don't have much to add to the whether/now/later question of providing
a "cp" replacement, but I guess the existing command-line options and
documentation wouldn't have to change with our own "cp" replacement
while the newly proposed '-h' and '-p' would become moot then, right?

Regards,
Martin