Re: [PATCHES] odd output in restore mode - Mailing list pgsql-hackers

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@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@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + If your life is a hard drive, Christ can be your backup. +
Index: pgstandby.sgml
===================================================================
RCS file: /cvsroot/pgsql/doc/src/sgml/pgstandby.sgml,v
retrieving revision 2.5
diff -c -r2.5 pgstandby.sgml
*** pgstandby.sgml    7 May 2008 18:48:40 -0000    2.5
--- pgstandby.sgml    15 Dec 2008 22:04:09 -0000
***************
*** 295,301 ****
    </itemizedlist>

    <para>
!    Since the Windows example uses <literal>copy</> at both ends, either
     or both servers might be accessing the archive directory across the
     network.
    </para>
--- 295,310 ----
    </itemizedlist>

    <para>
!    The <literal>copy</> command on Windows sets the final file size
!    before the file is completely copied, which would ordinarly confuse
!    <application>pg_standby</application>.  Therefore
!    <application>pg_standby</application> waits <literal>sleeptime</>
!    seconds once it sees the proper file size.  GNUWin32's <literal>cp</>
!    sets the file size only after the file copy is complete.
!   </para>
!
!   <para>
!    Using the Since the Windows example uses <literal>copy</> at both ends, either
     or both servers might be accessing the archive directory across the
     network.
    </para>