Re: [HACKERS] odd output in restore mode

[Simon Riggs]

On Mon, 2008-06-30 at 19:29 -0400, Andrew Dunstan wrote:

>
> Bruce Momjian wrote:
> > Dave Page wrote:
> >  
> >> On Sun, May 18, 2008 at 1:38 PM, Andrew Dunstan <andrew@...> wrote:
> >>    
> >>> I don't know how you tested,
> >>>      
> >> Copy a large file across a relatively slow network, and check the size
> >> on the destination drive before it finishes.
> >>
> >>    
> >>> but could you please repeat the test with
> >>> GnuWin32's cp.exe? If it doesn't preallocate the space then I think our way
> >>> forward is reasonably clear:
> >>>      
> >> It does not pre-allocate.
> >>
> >>    
> >>> . we recommend its use for Windows archive_command settings
> >>> . we provide the delay kluge as switchable behaviour on Windows instead of
> >>> having it always on.
> >>>      
> >> Sounds reasonable to me.
> >>    
> >
> > Are there any changes we need to make here?
> >
> >  
>
> Yes. Simon has promised a patch to do the above.

Patch implements

* recommendation to use GnuWin32 cp on Windows
* provide "holdtime" delay, default 0 (on all platforms)
* default stays same on Windows="copy" to ensure people upgrading don't
get stung

Patch should be backpatched to 8.3, plus to CVS HEAD.

We should recommend in next 8.3 release notes that people use "-p" or
"-l" rather than just letting it default.

Will add permalink to Wiki when patch appears in archives.

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

[pg_standby_win32.v1.patch]

---

Index: contrib/pg_standby/pg_standby.c
===================================================================
RCS file: /home/sriggs/pg/REPOSITORY/pgsql/contrib/pg_standby/pg_standby.c,v
retrieving revision 1.12
diff -c -r1.12 pg_standby.c
*** contrib/pg_standby/pg_standby.c 17 May 2008 01:28:21 -0000 1.12
--- contrib/pg_standby/pg_standby.c 1 Jul 2008 08:18:50 -0000
***************
*** 44,49 ****
--- 44,50 ----
 
  /* Options and defaults */
  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? */
***************
*** 67,75 ****
  char exclusiveCleanupFileName[MAXPGPATH]; /* the file we need to
  * get from archive */
 
! #define RESTORE_COMMAND_COPY 0
! #define RESTORE_COMMAND_LINK 1
! int restoreCommandType;
 
  #define XLOG_DATA 0
  #define XLOG_HISTORY 1
--- 68,83 ----
  char exclusiveCleanupFileName[MAXPGPATH]; /* the file we need to
  * get from archive */
 
! #define RESTORE_COMMAND_CP 0
! #define RESTORE_COMMAND_LN 1
! #define RESTORE_COMMAND_WIN32_MKLINK 2
! #define RESTORE_COMMAND_WIN32_COPY 3
! #ifdef WIN32
! int restoreCommandType = RESTORE_COMMAND_WIN32_COPY;
! #else
! int restoreCommandType = RESTORE_COMMAND_CP;
! #endif
!
 
  #define XLOG_DATA 0
  #define XLOG_HISTORY 1
***************
*** 113,142 ****
  {
  #ifdef WIN32
  snprintf(WALFilePath, MAXPGPATH, "%s\\%s", archiveLocation, nextWALFileName);
  switch (restoreCommandType)
  {
! case RESTORE_COMMAND_LINK:
  SET_RESTORE_COMMAND("mklink", WALFilePath, xlogFilePath);
! case RESTORE_COMMAND_COPY:
! default:
  SET_RESTORE_COMMAND("copy", WALFilePath, xlogFilePath);
  break;
! }
! #else
! snprintf(WALFilePath, MAXPGPATH, "%s/%s", archiveLocation, nextWALFileName);
! switch (restoreCommandType)
! {
! case RESTORE_COMMAND_LINK:
! #if HAVE_WORKING_LINK
  SET_RESTORE_COMMAND("ln -s -f", WALFilePath, xlogFilePath);
  break;
! #endif
! case RESTORE_COMMAND_COPY:
! default:
  SET_RESTORE_COMMAND("cp", WALFilePath, xlogFilePath);
  break;
  }
- #endif
 
  /*
  * This code assumes that archiveLocation is a directory You may wish to
--- 121,148 ----
  {
  #ifdef WIN32
  snprintf(WALFilePath, MAXPGPATH, "%s\\%s", archiveLocation, nextWALFileName);
+ #else
+ snprintf(WALFilePath, MAXPGPATH, "%s/%s", archiveLocation, nextWALFileName);
+ #endif
  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;
! case RESTORE_COMMAND_LN:
  SET_RESTORE_COMMAND("ln -s -f", WALFilePath, xlogFilePath);
  break;
! case RESTORE_COMMAND_CP:
  SET_RESTORE_COMMAND("cp", WALFilePath, xlogFilePath);
  break;
+ default:
+ fprintf(stderr, "pg_standby: unknown restore command\n");
+ fflush(stderr);
+ exit(2);
+ break;
  }
 
  /*
  * This code assumes that archiveLocation is a directory You may wish to
***************
*** 175,191 ****
  }
  else if (stat_buf.st_size == XLOG_SEG_SIZE)
  {
- #ifdef WIN32
-
  /*
! * Windows reports that the file has the right number of bytes
  * even though the file is still being copied and cannot be opened
! * by pg_standby yet. So we wait for sleeptime secs before
  * attempting to restore. If that is not enough, we will rely on
  * the retry/holdoff mechanism.
  */
! pg_usleep(sleeptime * 1000000L);
! #endif
  nextWALFileType = XLOG_DATA;
  return true;
  }
--- 181,196 ----
  }
  else if (stat_buf.st_size == XLOG_SEG_SIZE)
  {
  /*
! * Some utilities report that the file has the right number of bytes
  * even though the file is still being copied and cannot be opened
! * by pg_standby yet. So we wait for holdtime secs before
  * attempting to restore. If that is not enough, we will rely on
  * the retry/holdoff mechanism.
  */
! if (holdtime > 0)
! pg_usleep(holdtime * 1000000L);
!
  nextWALFileType = XLOG_DATA;
  return true;
  }
***************
*** 441,448 ****
--- 446,455 ----
  fprintf(stderr, "  -d generate lots of debugging output (testing only)\n");
  fprintf(stderr, "  -k NUMFILESTOKEEP if RESTARTWALFILE not used, removes files prior to limit (0 keeps all)\n");
  fprintf(stderr, "  -l links into archive (leaves file in archive)\n");
+ fprintf(stderr, "  -p always uses GNU compatible 'cp' command on all platforms\n");
  fprintf(stderr, "  -r MAXRETRIES max number of times to retry, with progressive wait (default=3)\n");
  fprintf(stderr, "  -s SLEEPTIME seconds to wait between file checks (min=1, max=60, default=5)\n");
+ fprintf(stderr, "  -h HOLDTIME seconds to wait once file appears full (min=0, max=60, default=0)\n");
  fprintf(stderr, "  -t TRIGGERFILE defines a trigger file to initiate failover (no default)\n");
  fprintf(stderr, "  -w MAXWAITTIME max seconds to wait for a file (0=no limit)(default=0)\n");
  fflush(stderr);
***************
*** 463,474 ****
  (void) signal(SIGINT, sighandler);
  (void) signal(SIGQUIT, sighandler);
 
! while ((c = getopt(argc, argv, "cdk:lr:s:t:w:")) != -1)
  {
  switch (c)
  {
  case 'c': /* Use copy */
! restoreCommandType = RESTORE_COMMAND_COPY;
  break;
  case 'd': /* Debug mode */
  debug = true;
--- 470,488 ----
  (void) signal(SIGINT, sighandler);
  (void) signal(SIGQUIT, sighandler);
 
! while ((c = getopt(argc, argv, "cdh:k:lpr:s:t:w:")) != -1)
  {
  switch (c)
  {
  case 'c': /* Use copy */
! #ifdef WIN32
! restoreCommandType = RESTORE_COMMAND_WIN32_COPY;
! #else
! restoreCommandType = RESTORE_COMMAND_CP;
! #endif
! break;
! case 'p': /* Use cp */
! restoreCommandType = RESTORE_COMMAND_CP;
  break;
  case 'd': /* Debug mode */
  debug = true;
***************
*** 483,489 ****
  }
  break;
  case 'l': /* Use link */
! restoreCommandType = RESTORE_COMMAND_LINK;
  break;
  case 'r': /* Retries */
  maxretries = atoi(optarg);
--- 497,511 ----
  }
  break;
  case 'l': /* Use link */
! #ifdef WIN32
! restoreCommandType = RESTORE_COMMAND_WIN32_MKLINK;
! #else
! #if HAVE_WORKING_LINK
! restoreCommandType = RESTORE_COMMAND_LN;
! #else
! restoreCommandType = RESTORE_COMMAND_CP;
! #endif
! #endif
  break;
  case 'r': /* Retries */
  maxretries = atoi(optarg);
***************
*** 503,508 ****
--- 525,539 ----
  exit(2);
  }
  break;
+ case 'h': /* Hold time */
+ holdtime = atoi(optarg);
+ if (holdtime < 0 || holdtime > 60)
+ {
+ fprintf(stderr, "usage: pg_standby -h holdtime incorrectly set\n");
+ usage();
+ exit(2);
+ }
+ break;
  case 't': /* Trigger file */
  triggerPath = optarg;
  if (CheckForExternalTrigger())
Index: doc/src/sgml/standby.sgml
===================================================================
RCS file: /home/sriggs/pg/REPOSITORY/pgsql/doc/src/sgml/standby.sgml,v
retrieving revision 1.1
diff -c -r1.1 standby.sgml
*** doc/src/sgml/standby.sgml 10 Nov 2007 23:30:46 -0000 1.1
--- doc/src/sgml/standby.sgml 1 Jul 2008 08:29:16 -0000
***************
*** 136,141 ****
--- 136,149 ----
        </entry>
       </row>
       <row>
+       <entry>-p</entry>
+       <entry>
+        <para>
+         always use cp command on any platform
+        </para>
+       </entry>
+      </row>
+      <row>
        <entry>-r maxretries</entry>
        <entry>
         <para>
***************
*** 160,165 ****
--- 168,182 ----
        </entry>
       </row>
       <row>
+       <entry>-h holdtime</entry>
+       <entry>
+        the number of seconds to wait once a file appears full.
+   This is required with some utilities, such as Win32 Copy, which
+   sets the filesize before it has completed the copy process.
+        <literal>Default=0</literal>
+       </entry>
+      </row>
+      <row>
        <entry>-t triggerfile</entry>
        <entry>
         the presence of the triggerfile will cause recovery to end
***************
*** 228,234 ****
       *not* in the restore_command, in 8.2, 8.1, 8.0 on Windows.
      </para>
      <programlisting>
! restore_command = 'pg_standby -c -d -s 5 -w 0 -t C:\pgsql.trigger.5442
   ..\archive %f %p 2>> standby.log'
      </programlisting>
      <para>
--- 245,251 ----
       *not* in the restore_command, in 8.2, 8.1, 8.0 on Windows.
      </para>
      <programlisting>
! restore_command = 'pg_standby -c -d -s 5 -h 5 -w 0 -t C:\pgsql.trigger.5442
   ..\archive %f %p 2>> standby.log'
      </programlisting>
      <para>
***************
*** 238,248 ****
--- 255,272 ----
       <listitem><para>use a copy command to restore WAL files from archive</para></listitem>
       <listitem><para>produce logfile output in standby.log</para></listitem>
       <listitem><para>sleep for 5 seconds between checks for next WAL file is full</para></listitem>
+      <listitem><para>sleep for 5 second after the file appears full</para></listitem>
       <listitem><para>never timeout if file not found</para></listitem>
       <listitem><para>stop waiting when a trigger file called C:\pgsql.trigger.5442 appears</para></listitem>
      </itemizedlist>
     </listitem>
    </itemizedlist>
+     <para>
+      On Windows you are advised to use GNUWin32's cp utility. The cp utility
+ does not set filesize until file copying is complete, so there is no
+ need to use the -h option to introduce additional delays to ensure
+ correctness. See <ulink url="http://gnuwin32.sourceforge.net/"></ulink>
+     </para>
   </sect2>
   
  </sect1>

---

--
Sent via pgsql-patches mailing list (pgsql-patches@...)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches

[Heikki Linnakangas-2]

Simon Riggs wrote:
> Patch implements
>
> * recommendation to use GnuWin32 cp on Windows
> * provide "holdtime" delay, default 0 (on all platforms)
> * default stays same on Windows="copy" to ensure people upgrading don't
> get stung

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.

(Sorry for jumping into the discussion so late, I didn't follow this
thread earlier, and just read it now in the archives while looking at
the patch.)

--
   Heikki Linnakangas
   EnterpriseDB   http://www.enterprisedb.com

--
Sent via pgsql-patches mailing list (pgsql-patches@...)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches

[Simon Riggs]

On Tue, 2008-07-01 at 13:44 +0300, Heikki Linnakangas wrote:

> Simon Riggs wrote:
> > Patch implements
> >
> > * recommendation to use GnuWin32 cp on Windows
> > * provide "holdtime" delay, default 0 (on all platforms)
> > * default stays same on Windows="copy" to ensure people upgrading don't
> > get stung
>
> 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.
>
> (Sorry for jumping into the discussion so late, I didn't follow this
> thread earlier, and just read it now in the archives while looking at
> the patch.)

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.

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


--
Sent via pgsql-patches mailing list (pgsql-patches@...)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches

[Bruce Momjian-5]

Simon Riggs wrote:

> > > * recommendation to use GnuWin32 cp on Windows
> > > * provide "holdtime" delay, default 0 (on all platforms)
> > > * default stays same on Windows="copy" to ensure people upgrading don't
> > > get stung
> >
> > 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.
> >
> > (Sorry for jumping into the discussion so late, I didn't follow this
> > thread earlier, and just read it now in the archives while looking at
> > the patch.)
>
> 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.

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

--
  Bruce Momjian  <bruce@...>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

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

--
Sent via pgsql-patches mailing list (pgsql-patches@...)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches

[Martin Zaun]

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

--
Sent via pgsql-patches mailing list (pgsql-patches@...)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches

[Simon Riggs]

On Tue, 2008-07-22 at 17:19 -0700, Martin Zaun wrote:
> 1. Issues with applying the patch to CVS HEAD:

Sounds awful. Thanks for the review, will fix.

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


--
Sent via pgsql-patches mailing list (pgsql-patches@...)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches

[Simon Riggs]

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


--
Sent via pgsql-patches mailing list (pgsql-patches@...)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches

[Heikki Linnakangas-2]

Simon Riggs wrote:

> On Tue, 2008-07-22 at 17:19 -0700, Martin Zaun wrote:
>> 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.

Hmm. I just realized that replacing the "cp" command within pg_standby
won't help at all. The problem is with the command that copies the files
*to* the archivelocation that pg_standby polls, not with the copy
pg_standby does from archivelocation to pg_xlog. And we don't have much
control over that.

We really need a more reliable way of detecting that a file has been
fully copied. One simple improvement would be to check the xlp_magic
field of the last page, though it still wouldn't be bullet-proof.

Do the commands that preallocate the space keep the file exclusively
locked during the copy? If they do, shouldn't we get an error in trying
to run the restore copy command, and retry after the 1s sleep in
RestoreWALFileForRecovery? Though if the archive location is a samba
mount or something, I guess we can't rely on Windows-style exclusive
locking.

--
   Heikki Linnakangas
   EnterpriseDB   http://www.enterprisedb.com

--
Sent via pgsql-patches mailing list (pgsql-patches@...)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches

[Kevin Grittner]

>>> "Heikki Linnakangas" <heikki@...> wrote:
 
> We really need a more reliable way of detecting that a file has been

> fully copied.
 
In our scripts we handle this by copying to a temp directory on the
same mount point as the archive directory and doing a mv to the
archive location when the copy is successfully completed.  I think
that this even works on Windows.  Could that just be documented as a
strong recommendation for the archive script?
 
-Kevin

--
Sent via pgsql-patches mailing list (pgsql-patches@...)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches

[Andrew Dunstan]

Kevin Grittner wrote:

>>>> "Heikki Linnakangas" <heikki@...> wrote:
>>>>        
>  
>  
>> We really need a more reliable way of detecting that a file has been
>>    
>
>  
>> fully copied.
>>    
>  
> In our scripts we handle this by copying to a temp directory on the
> same mount point as the archive directory and doing a mv to the
> archive location when the copy is successfully completed.  I think
> that this even works on Windows.  Could that just be documented as a
> strong recommendation for the archive script?
>  
>
>
>  

Needs testing at least. If it does in fact work then we can just adjust
the docs and be done - or maybe provide a .bat file or perl script that
would work as na archive_command on Windows.

cheers

andrew

--
Sent via pgsql-patches mailing list (pgsql-patches@...)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches

[Simon Riggs]

On Wed, 2008-07-23 at 21:38 +0300, Heikki Linnakangas wrote:

> Simon Riggs wrote:
> > On Tue, 2008-07-22 at 17:19 -0700, Martin Zaun wrote:
> >> 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.
>
> Hmm. I just realized that replacing the "cp" command within pg_standby
> won't help at all. The problem is with the command that copies the files
> *to* the archivelocation that pg_standby polls, not with the copy
> pg_standby does from archivelocation to pg_xlog. And we don't have much
> control over that.
>
> We really need a more reliable way of detecting that a file has been
> fully copied. One simple improvement would be to check the xlp_magic
> field of the last page, though it still wouldn't be bullet-proof.
>
> Do the commands that preallocate the space keep the file exclusively
> locked during the copy? If they do, shouldn't we get an error in trying
> to run the restore copy command, and retry after the 1s sleep in
> RestoreWALFileForRecovery? Though if the archive location is a samba
> mount or something, I guess we can't rely on Windows-style exclusive
> locking.

With respect, I need to refer you back to the my last paragraph above.

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


--
Sent via pgsql-patches mailing list (pgsql-patches@...)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches

[Simon Riggs]

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

> reviewing your patch

Current status is this:

* My understanding is that Dave and Andrew (and therefore Simon) think
the approach proposed here is an acceptable one. Heikki disagrees and
wants different approach. Perhaps I misunderstand.

* Patch needs work to complete the proposed approach

* I'm willing to change the patch, but not able to test it on Windows.

Is there someone able to test the patch, if I make the changes? If not,
we should just kick this out of the CommitFest queue now and be done. If
nobody cares enough about this issue to test a fix, we shouldn't bother.

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


--
Sent via pgsql-patches mailing list (pgsql-patches@...)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches

[Tom Lane-2]

Simon Riggs <simon@...> writes:
> On Tue, 2008-07-22 at 17:19 -0700, Martin Zaun wrote:
>> reviewing your patch

> Current status is this:
> * My understanding is that Dave and Andrew (and therefore Simon) think
> the approach proposed here is an acceptable one. Heikki disagrees and
> wants different approach. Perhaps I misunderstand.
> * Patch needs work to complete the proposed approach
> * I'm willing to change the patch, but not able to test it on Windows.

I thought the latest conclusion was that changing the behavior of
pg_standby itself wouldn't address the problem anyway, and that what we
need is just a docs patch recommending that people use safe copying
methods in their scripts that copy to the archive area?

                        regards, tom lane

--
Sent via pgsql-patches mailing list (pgsql-patches@...)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches

[Simon Riggs]

On Fri, 2008-07-25 at 16:31 -0400, Tom Lane wrote:

> Simon Riggs <simon@...> writes:
> > On Tue, 2008-07-22 at 17:19 -0700, Martin Zaun wrote:
> >> reviewing your patch
>
> > Current status is this:
> > * My understanding is that Dave and Andrew (and therefore Simon) think
> > the approach proposed here is an acceptable one. Heikki disagrees and
> > wants different approach. Perhaps I misunderstand.
> > * Patch needs work to complete the proposed approach
> > * I'm willing to change the patch, but not able to test it on Windows.
>
> I thought the latest conclusion was that changing the behavior of
> pg_standby itself wouldn't address the problem anyway, and that what we
> need is just a docs patch recommending that people use safe copying
> methods in their scripts that copy to the archive area?

Plus the rest of this patch, which is really very simple.

pg_standby currently waits (on Windows) for the sleep time. We agreed
that this sleep would be on by default, but optional.

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


--
Sent via pgsql-patches mailing list (pgsql-patches@...)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches

[Tom Lane-2]

Simon Riggs <simon@...> writes:
> On Fri, 2008-07-25 at 16:31 -0400, Tom Lane wrote:
>> I thought the latest conclusion was that changing the behavior of
>> pg_standby itself wouldn't address the problem anyway, and that what we
>> need is just a docs patch recommending that people use safe copying
>> methods in their scripts that copy to the archive area?

> Plus the rest of this patch, which is really very simple.

Why?  AFAICT the patch is just a kluge that adds user-visible complexity
without providing a solution that's actually sure to work.

                        regards, tom lane

--
Sent via pgsql-patches mailing list (pgsql-patches@...)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches

[Simon Riggs]

On Fri, 2008-07-25 at 16:58 -0400, Tom Lane wrote:

> Simon Riggs <simon@...> writes:
> > On Fri, 2008-07-25 at 16:31 -0400, Tom Lane wrote:
> >> I thought the latest conclusion was that changing the behavior of
> >> pg_standby itself wouldn't address the problem anyway, and that what we
> >> need is just a docs patch recommending that people use safe copying
> >> methods in their scripts that copy to the archive area?
>
> > Plus the rest of this patch, which is really very simple.
>
> Why?  AFAICT the patch is just a kluge that adds user-visible complexity
> without providing a solution that's actually sure to work.

First, I'm not the one objecting to the current behaviour.

Currently, there is a wait in there that can be removed if you use a
copy utility that sets size after it does a copy. So we agreed to make
it optional (at PGCon).

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


--
Sent via pgsql-patches mailing list (pgsql-patches@...)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches

[Heikki Linnakangas-2]

Andrew Dunstan wrote:

> Kevin Grittner wrote:
>>>>> "Heikki Linnakangas" <heikki@...> wrote:        
>>> We really need a more reliable way of detecting that a file has been
>>> fully copied.    
>>  
>> In our scripts we handle this by copying to a temp directory on the
>> same mount point as the archive directory and doing a mv to the
>> archive location when the copy is successfully completed.  I think
>> that this even works on Windows.  Could that just be documented as a
>> strong recommendation for the archive script?
>
> Needs testing at least. If it does in fact work then we can just adjust
> the docs and be done

Yeah.

> - or maybe provide a .bat file or perl script that
> would work as na archive_command on Windows.

We're not talking about archive_command. We're talking about the thing
that copies files to the directory that pg_standby polls.

--
   Heikki Linnakangas
   EnterpriseDB   http://www.enterprisedb.com

--
Sent via pgsql-patches mailing list (pgsql-patches@...)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches

[Andrew Dunstan]

Heikki Linnakangas wrote:
> Andrew Dunstan wrote:
>
>
>> - or maybe provide a .bat file or perl script that would work as na
>> archive_command on Windows.
>
> We're not talking about archive_command. We're talking about the thing
> that copies files to the directory that pg_standby polls.

Er, that's what the archive_command is. Look at the pg_standby docs and
you'll see that that's where we're currently recommending use of windows
copy. Perhaps you're confusing this with the restore_command?

cheers

andrew

--
Sent via pgsql-patches mailing list (pgsql-patches@...)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches

[Heikki Linnakangas-2]

Andrew Dunstan wrote:

>
>
> Heikki Linnakangas wrote:
>> Andrew Dunstan wrote:
>>
>>
>>> - or maybe provide a .bat file or perl script that would work as na
>>> archive_command on Windows.
>>
>> We're not talking about archive_command. We're talking about the thing
>> that copies files to the directory that pg_standby polls.
>
> Er, that's what the archive_command is. Look at the pg_standby docs and
> you'll see that that's where we're currently recommending use of windows
> copy. Perhaps you're confusing this with the restore_command?

Oh, right. I was thinking that archive_command copies the files to an
archive location, and there's yet another process copying files from
there to the directory pg_standby polls. But indeed in the simple
configuration, archive_command is the command that we're interested in.

--
   Heikki Linnakangas
   EnterpriseDB   http://www.enterprisedb.com

--
Sent via pgsql-patches mailing list (pgsql-patches@...)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches

[Greg Smith-12]

On Wed, 23 Jul 2008, Kevin Grittner wrote:

> In our scripts we handle this by copying to a temp directory on the
> same mount point as the archive directory and doing a mv to the
> archive location when the copy is successfully completed.  I think
> that this even works on Windows.  Could that just be documented as a
> strong recommendation for the archive script?

This is exactly what I always do.  I think the way cp is shown in the
examples promotes what's really a bad practice for lots of reasons, this
particular problem being just one of them.

I've been working on an improved archive_command shell script that I
expect to submit for comments and potential inclusion in the documentation
as a better base for other people to build on.  This is one of the options
for how it can operate.  It would be painful but not impossible to convert
a subset of that script to run under Windows as well, at least enough to
cover this particular issue.

--
* Greg Smith gsmith@... http://www.gregsmith.com Baltimore, MD

--
Sent via pgsql-patches mailing list (pgsql-patches@...)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches