Re: [PATCHES] odd output in restore mode

Since this patch was rejected, I have added the attached documentation
to pg_standby to mention the sleep() we do.

---------------------------------------------------------------------------

Martin Zaun wrote:
>
> 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

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +