Re: [HACKERS] Still another race condition in recovery TAP tests - Mailing list pgsql-hackers

Michael Paquier <michael.paquier@gmail.com> writes:
> On Sun, Sep 10, 2017 at 12:38 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> The specific case we need to allow is "ENOENT on a file/dir that was
>> there a moment ago".  I think it still behooves us to complain about
>> anything else.  If you think it's a simple fix, have at it.  But
>> I see at least three ways for _copypath_recurse to fail depending on
>> exactly when the file disappears.

> With the check for -d and -f, this brings indeed the count to three
> code paths. With the patch attached, I have added some manual sleep
> calls in RecursiveCopy.pm and triggered manual deletions of the source
> repository. The copy is able to complete in any case, warning about
> missing directories or files. I have added warn messages in the patch
> when ENOENT is triggered, though I think that those should be removed
> in the final patch.

Hm, I don't much like having it silently ignore files that are present;
that seems like a foot-gun in the long run.  What do you think of the
attached?

> By the way, 010_logical_decoding_timelines.pl does not need to include
> RecursiveCopy.

Good catch.

            regards, tom lane

diff --git a/src/test/perl/RecursiveCopy.pm b/src/test/perl/RecursiveCopy.pm
index 28ecaf6..19f7dd2 100644
*** a/src/test/perl/RecursiveCopy.pm
--- b/src/test/perl/RecursiveCopy.pm
*************** use File::Copy;
*** 29,40 ****
  =head2 copypath($from, $to, %params)

  Recursively copy all files and directories from $from to $to.

  Only regular files and subdirectories are copied.  Trying to copy other types
  of directory entries raises an exception.

  Raises an exception if a file would be overwritten, the source directory can't
! be read, or any I/O operation fails. Always returns true.

  If the B<filterfn> parameter is given, it must be a subroutine reference.
  This subroutine will be called for each entry in the source directory with its
--- 29,45 ----
  =head2 copypath($from, $to, %params)

  Recursively copy all files and directories from $from to $to.
+ Does not preserve file metadata (e.g., permissions).

  Only regular files and subdirectories are copied.  Trying to copy other types
  of directory entries raises an exception.

  Raises an exception if a file would be overwritten, the source directory can't
! be read, or any I/O operation fails.  However, we silently ignore ENOENT on
! open, because when copying from a live database it's possible for a file/dir
! to be deleted after we see its directory entry but before we can open it.
!
! Always returns true.

  If the B<filterfn> parameter is given, it must be a subroutine reference.
  This subroutine will be called for each entry in the source directory with its
*************** sub copypath
*** 74,79 ****
--- 79,87 ----
          $filterfn = sub { return 1; };
      }

+     # Complain if original path is bogus, because _copypath_recurse won't.
+     die "\"$base_src_dir\" does not exist" if !-e $base_src_dir;
+
      # Start recursive copy from current directory
      return _copypath_recurse($base_src_dir, $base_dest_dir, "", $filterfn);
  }
*************** sub _copypath_recurse
*** 89,100 ****
      return 1 unless &$filterfn($curr_path);

      # Check for symlink -- needed only on source dir
!     die "Cannot operate on symlinks" if -l $srcpath;
!
!     # Can't handle symlinks or other weird things
!     die "Source path \"$srcpath\" is not a regular file or directory"
!       unless -f $srcpath
!           or -d $srcpath;

      # Abort if destination path already exists.  Should we allow directories
      # to exist already?
--- 97,104 ----
      return 1 unless &$filterfn($curr_path);

      # Check for symlink -- needed only on source dir
!     # (note: this will fall through quietly if file is already gone)
!     die "Cannot operate on symlink \"$srcpath\"" if -l $srcpath;

      # Abort if destination path already exists.  Should we allow directories
      # to exist already?
*************** sub _copypath_recurse
*** 104,128 ****
      # same name and we're done.
      if (-f $srcpath)
      {
!         copy($srcpath, $destpath)
            or die "copy $srcpath -> $destpath failed: $!";
          return 1;
      }

!     # Otherwise this is directory: create it on dest and recurse onto it.
!     mkdir($destpath) or die "mkdir($destpath) failed: $!";
!
!     opendir(my $directory, $srcpath) or die "could not opendir($srcpath): $!";
!     while (my $entry = readdir($directory))
      {
!         next if ($entry eq '.' or $entry eq '..');
!         _copypath_recurse($base_src_dir, $base_dest_dir,
!             $curr_path eq '' ? $entry : "$curr_path/$entry", $filterfn)
!           or die "copypath $srcpath/$entry -> $destpath/$entry failed";
      }
-     closedir($directory);

!     return 1;
  }

  1;
--- 108,154 ----
      # same name and we're done.
      if (-f $srcpath)
      {
!         my $fh;
!         unless (open($fh, '<', $srcpath))
!         {
!             return 1 if ($!{ENOENT});
!             die "open($srcpath) failed: $!";
!         }
!         copy($fh, $destpath)
            or die "copy $srcpath -> $destpath failed: $!";
+         close $fh;
          return 1;
      }

!     # If it's a directory, create it on dest and recurse into it.
!     if (-d $srcpath)
      {
!         my $directory;
!         unless (opendir($directory, $srcpath))
!         {
!             return 1 if ($!{ENOENT});
!             die "opendir($srcpath) failed: $!";
!         }
!
!         mkdir($destpath) or die "mkdir($destpath) failed: $!";
!
!         while (my $entry = readdir($directory))
!         {
!             next if ($entry eq '.' or $entry eq '..');
!             _copypath_recurse($base_src_dir, $base_dest_dir,
!                 $curr_path eq '' ? $entry : "$curr_path/$entry", $filterfn)
!               or die "copypath $srcpath/$entry -> $destpath/$entry failed";
!         }
!
!         closedir($directory);
!         return 1;
      }

!     # If it disappeared from sight, that's OK.
!     return 1 if !-e $srcpath;
!
!     # Else it's some weird file type; complain.
!     die "Source path \"$srcpath\" is not a regular file or directory";
  }

  1;
diff --git a/src/test/recovery/t/010_logical_decoding_timelines.pl
b/src/test/recovery/t/010_logical_decoding_timelines.pl
index edc0219..5620450 100644
*** a/src/test/recovery/t/010_logical_decoding_timelines.pl
--- b/src/test/recovery/t/010_logical_decoding_timelines.pl
*************** use warnings;
*** 24,30 ****
  use PostgresNode;
  use TestLib;
  use Test::More tests => 13;
- use RecursiveCopy;
  use File::Copy;
  use IPC::Run ();
  use Scalar::Util qw(blessed);
--- 24,29 ----

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