TAP tests aren't using the magic words for Windows file access
Buildfarm member drongo has been failing the pg_ctl regression test
pretty often. I happened to look closer at what's happening, and
it's this:
could not read "C:/prog/bf/root/HEAD/pgsql.build/src/bin/pg_ctl/tmp_check/t_004_logrotate_primary_data/pgdata/current_logfiles": Permission denied at C:/prog/bf/root/HEAD/pgsql.build/src/test/perl/TestLib.pm line 397.
That is, TestLib::slurp_file is failing to read a file. Almost
certainly, "permission denied" doesn't really mean a permissions
problem, but failure to specify the file-opening flags needed to
allow concurrent access on Windows. We fixed this in pg_ctl
itself in commit 0ba06e0bf ... but we didn't fix the TAP
infrastructure. Is there an easy way to get Perl on board
with that?
regards, tom lane
On Sun, Nov 03, 2019 at 10:53:00PM -0500, Tom Lane wrote:
That is, TestLib::slurp_file is failing to read a file. Almost
certainly, "permission denied" doesn't really mean a permissions
problem, but failure to specify the file-opening flags needed to
allow concurrent access on Windows. We fixed this in pg_ctl
itself in commit 0ba06e0bf ... but we didn't fix the TAP
infrastructure. Is there an easy way to get Perl on board
with that?
If we were to use Win32API::File so as the file is opened in shared
mode, we would do the same as what our frontend/backend code does (see
$uShare):
https://metacpan.org/pod/Win32API::File
--
Michael
On 11/4/19 10:41 PM, Michael Paquier wrote:
On Sun, Nov 03, 2019 at 10:53:00PM -0500, Tom Lane wrote:
That is, TestLib::slurp_file is failing to read a file. Almost
certainly, "permission denied" doesn't really mean a permissions
problem, but failure to specify the file-opening flags needed to
allow concurrent access on Windows. We fixed this in pg_ctl
itself in commit 0ba06e0bf ... but we didn't fix the TAP
infrastructure. Is there an easy way to get Perl on board
with that?If we were to use Win32API::File so as the file is opened in shared
mode, we would do the same as what our frontend/backend code does (see
$uShare):
https://metacpan.org/pod/Win32API::File
Hmm. What would that look like? (My eyes glazed over a bit reading that
page - probably ENOTENOUGHCAFFEINE)
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2019-Nov-05, Michael Paquier wrote:
On Sun, Nov 03, 2019 at 10:53:00PM -0500, Tom Lane wrote:
That is, TestLib::slurp_file is failing to read a file. Almost
certainly, "permission denied" doesn't really mean a permissions
problem, but failure to specify the file-opening flags needed to
allow concurrent access on Windows. We fixed this in pg_ctl
itself in commit 0ba06e0bf ... but we didn't fix the TAP
infrastructure. Is there an easy way to get Perl on board
with that?If we were to use Win32API::File so as the file is opened in shared
mode, we would do the same as what our frontend/backend code does (see
$uShare):
https://metacpan.org/pod/Win32API::File
Compatibility-wise, that should be okay, since that module appears to
have been distributed with Perl core early on.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Wed, Nov 6, 2019 at 4:38 PM Alvaro Herrera <alvherre@2ndquadrant.com>
wrote:
On 2019-Nov-05, Michael Paquier wrote:
On Sun, Nov 03, 2019 at 10:53:00PM -0500, Tom Lane wrote:
That is, TestLib::slurp_file is failing to read a file. Almost
certainly, "permission denied" doesn't really mean a permissions
problem, but failure to specify the file-opening flags needed to
allow concurrent access on Windows. We fixed this in pg_ctl
itself in commit 0ba06e0bf ... but we didn't fix the TAP
infrastructure. Is there an easy way to get Perl on board
with that?If we were to use Win32API::File so as the file is opened in shared
mode, we would do the same as what our frontend/backend code does (see
$uShare):
https://metacpan.org/pod/Win32API::FileCompatibility-wise, that should be okay, since that module appears to
have been distributed with Perl core early on.
Please find attached a patch that adds the FILE_SHARE options to
TestLib::slurp_file using Win32API::File.
Regards,
Juan José Santamaría Flecha
Attachments:
0001-tap-file_share-windows-file-access.patchapplication/x-patch; name=0001-tap-file_share-windows-file-access.patchDownload
diff --git a/src/test/perl/TestLib.pm b/src/test/perl/TestLib.pm
index 905d0d1..25ab1f2 100644
--- a/src/test/perl/TestLib.pm
+++ b/src/test/perl/TestLib.pm
@@ -112,6 +112,22 @@ BEGIN
# Must be set early
$windows_os = $Config{osname} eq 'MSWin32' || $Config{osname} eq 'msys';
+ if ($windows_os)
+ {
+ require Win32::API;
+ Win32::API->import;
+
+ require Win32API::File;
+ Win32API::File->import(qw(
+ :Func
+ :Misc
+ :FILE_
+ :FILE_FLAG_
+ :FILE_SHARE_
+ :FILE_ATTRIBUTE_
+ :GENERIC_
+ ));
+ }
}
=pod
@@ -394,10 +410,25 @@ sub slurp_file
{
my ($filename) = @_;
local $/;
- open(my $in, '<', $filename)
- or die "could not read \"$filename\": $!";
- my $contents = <$in>;
- close $in;
+ my $contents;
+ if (!$windows_os)
+ {
+ open(my $in, '<', $filename)
+ or die "could not read \"$filename\": $!";
+ $contents = <$in>;
+ close $in;
+ }
+ else
+ {
+ my $fHandle = CreateFile($filename, GENERIC_READ(),
+ FILE_SHARE_DELETE()|FILE_SHARE_READ()|FILE_SHARE_WRITE(), [], OPEN_EXISTING(), 0, [])
+ or die "could not read \"$filename\": $^E";
+ OsFHandleOpen(my $fh = IO::Handle->new(), $fHandle, 'r')
+ or die "OsFHandleOpen: $^E\n";
+ $contents = <$fh>;
+ CloseHandle($fHandle)
+ or die "CloseHandle: $^E\n";
+ }
$contents =~ s/\r//g if $Config{osname} eq 'msys';
return $contents;
}
=?UTF-8?Q?Juan_Jos=C3=A9_Santamar=C3=ADa_Flecha?= <juanjo.santamaria@gmail.com> writes:
Please find attached a patch that adds the FILE_SHARE options to
TestLib::slurp_file using Win32API::File.
Ick. Are we going to need Windows-droppings like this all over the
TAP tests? I'm not sure I believe that slurp_file is the only place
with a problem.
regards, tom lane
On Thu, Nov 7, 2019 at 10:43 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
=?UTF-8?Q?Juan_Jos=C3=A9_Santamar=C3=ADa_Flecha?= <juanjo.santamaria@gmail.com> writes:
Please find attached a patch that adds the FILE_SHARE options to
TestLib::slurp_file using Win32API::File.Ick. Are we going to need Windows-droppings like this all over the
TAP tests? I'm not sure I believe that slurp_file is the only place
with a problem.
Not a Windows or Perl person, but I see that you can redefine core
functions with *CORE::GLOBAL::open = <replacement/wrapper>, if you
wanted to make a version of open() that does that FILE_SHARE_READ
dance. Alternatively we could of course have our own xxx_open()
function and use that everywhere, but that might be more distracting.
I'm a bit surprised that there doesn't seem to be a global switch
thing you can set somewhere to make it do that anyway. Doesn't
everyone eventually figure out that all files really want to be
shared?
On 11/6/19 4:43 PM, Tom Lane wrote:
=?UTF-8?Q?Juan_Jos=C3=A9_Santamar=C3=ADa_Flecha?= <juanjo.santamaria@gmail.com> writes:
Please find attached a patch that adds the FILE_SHARE options to
TestLib::slurp_file using Win32API::File.Ick. Are we going to need Windows-droppings like this all over the
TAP tests? I'm not sure I believe that slurp_file is the only place
with a problem.
In any case, the patch will fail as written - on the Msys 1 system I
just tested Win32::API is not available to the DTK perl we need to use
to run TAP tests.
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Thu, Nov 07, 2019 at 11:13:32AM +1300, Thomas Munro wrote:
Not a Windows or Perl person, but I see that you can redefine core
functions with *CORE::GLOBAL::open = <replacement/wrapper>, if you
wanted to make a version of open() that does that FILE_SHARE_READ
dance.
FWIW, I would have gone with a solution like that, say within
TestLib.pm's INIT. This ensures that any new future tests don't fall
into that trap again.
Alternatively we could of course have our own xxx_open() function
and use that everywhere, but that might be more distracting.
That does not sound really appealing.
I'm a bit surprised that there doesn't seem to be a global switch
thing you can set somewhere to make it do that anyway. Doesn't
everyone eventually figure out that all files really want to be
shared?
I guess it depends on your requirements. Looking around I can see
some mention about flock() but it does not solve the problem at the
time the fd is opened. If this does not exist, then it seems to me
that we have very special requirements for our perl code, and that
these are not popular.
--
Michael
On Thu, Nov 7, 2019 at 1:57 AM Andrew Dunstan <
andrew.dunstan@2ndquadrant.com> wrote:
In any case, the patch will fail as written - on the Msys 1 system I
just tested Win32::API is not available to the DTK perl we need to use
to run TAP tests.
May I ask which version of Msys is that system using? In a recent
installation (post 1.0.11) I see that those modules are available.
Regards,
Juan José Santamaría Flecha
On 11/7/19 3:42 AM, Juan José Santamaría Flecha wrote:
On Thu, Nov 7, 2019 at 1:57 AM Andrew Dunstan
<andrew.dunstan@2ndquadrant.com
<mailto:andrew.dunstan@2ndquadrant.com>> wrote:In any case, the patch will fail as written - on the Msys 1 system I
just tested Win32::API is not available to the DTK perl we need to use
to run TAP tests.May I ask which version of Msys is that system using? In a recent
installation (post 1.0.11) I see that those modules are available.
Not sure how I discover that. The path is c:\mingw\msys\1.0, looks like
it was installed in 2013 some time. perl reports version 5.8.8 built for
msys-int64
This is the machine that runs jacana on the buildfarm.
The test I'm running is:
perl -MWin32::API -e ';'
And perl reports it can't find the module.
However, the perl on my pretty recent Msys2 system (the one that runs
fairywren) reports the same problem. That's 5.30.0 built for
x86_64-msys-thread-multi.
So my question is which perl you're testing with? If it's a Windows
native perl version such as ActivePerl or StrawberryPerl that won't do -
the buildfarm needs to use msys-perl to run prove.
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2019-Nov-07, Andrew Dunstan wrote:
The test I'm running is:
��� perl -MWin32::API -e ';'
And perl reports it can't find the module.
That's a curious test to try, given that the module is called
Win32API::File.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 11/7/19 8:53 AM, Alvaro Herrera wrote:
On 2019-Nov-07, Andrew Dunstan wrote:
The test I'm running is:
perl -MWin32::API -e ';'
And perl reports it can't find the module.
That's a curious test to try, given that the module is called
Win32API::File.
The patch says:
+ require Win32::API;
+ Win32::API->import;
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2019-Nov-07, Andrew Dunstan wrote:
On 11/7/19 8:53 AM, Alvaro Herrera wrote:
That's a curious test to try, given that the module is called
Win32API::File.The patch says:
+��� ��� require Win32::API;
+��� ��� Win32::API->import;
Oh, you're right, it does. I wonder why, though:
$ corelist -a Win32::API
Data for 2018-11-29
Win32::API was not in CORE (or so I think)
$ corelist -a Win32API::File
Data for 2018-11-29
Win32API::File was first released with perl v5.8.9
v5.8.9 0.1001_01
v5.9.4 0.1001
v5.9.5 0.1001_01
v5.10.0 0.1001_01
...
According to the Win32API::File manual, you can request a file to be
shared by passing the string "r" to svShare to method createFile().
So do we really need all those extremely ugly "droppings" Juanjo added
to the patch?
(BTW the Win32API::File manual also says this:
"The default for $svShare is "rw" which provides the same sharing as
using regular perl open()."
I wonder why "the regular perl open()" is not doing the sharing thing
correctly ... has the problem has been misdiagnosed?).
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 11/7/19 9:12 AM, Alvaro Herrera wrote:
On 2019-Nov-07, Andrew Dunstan wrote:
On 11/7/19 8:53 AM, Alvaro Herrera wrote:
That's a curious test to try, given that the module is called
Win32API::File.The patch says:
+ require Win32::API;
+ Win32::API->import;Oh, you're right, it does. I wonder why, though:
$ corelist -a Win32::API
Data for 2018-11-29
Win32::API was not in CORE (or so I think)$ corelist -a Win32API::File
Data for 2018-11-29
Win32API::File was first released with perl v5.8.9
v5.8.9 0.1001_01
v5.9.4 0.1001
v5.9.5 0.1001_01
v5.10.0 0.1001_01
...
Yes, that's present on jacana and fairywren (not on frogmouth, which is
running a very old perl, but it doesn't run TAP tests anyway.)
According to the Win32API::File manual, you can request a file to be
shared by passing the string "r" to svShare to method createFile().
So do we really need all those extremely ugly "droppings" Juanjo added
to the patch?(BTW the Win32API::File manual also says this:
"The default for $svShare is "rw" which provides the same sharing as
using regular perl open()."
I wonder why "the regular perl open()" is not doing the sharing thing
correctly ... has the problem has been misdiagnosed?).
Maybe we need "rwd"?
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 11/7/19 9:12 AM, Alvaro Herrera wrote:
The patch says:
+ require Win32::API;
+ Win32::API->import;Oh, you're right, it does. I wonder why, though:
On further inspection I think those lines are unnecessary. The remainder
of the patch doesn't use this at all, AFAICT.
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Nov 8, 2019 at 3:41 AM Andrew Dunstan
<andrew.dunstan@2ndquadrant.com> wrote:
On 11/7/19 9:12 AM, Alvaro Herrera wrote:
The patch says:
+ require Win32::API;
+ Win32::API->import;Oh, you're right, it does. I wonder why, though:
On further inspection I think those lines are unnecessary. The remainder
of the patch doesn't use this at all, AFAICT.
So does that mean we're back on, we can use a patch like Juan Jose's?
I'd love to get rid of these intermittent buildfarm failures, like
this one just now:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=drongo&dt=2019-11-20%2010%3A00%3A10
Here you can see:
could not read "C:/prog/bf/root/HEAD/pgsql.build/src/bin/pg_ctl/tmp_check/t_004_logrotate_primary_data/pgdata/current_logfiles":
Permission denied at
C:/prog/bf/root/HEAD/pgsql.build/src/test/perl/TestLib.pm line 397.
That line is in the subroutine slurp_file, and says open(my $in, '<',
$filename). Using various clues from this thread, it seems like we
could, on Windows only, add code to TestLib.pm's INIT to rebind
*CORE::GLOBAL::open to a wrapper function that would just do
CreateFile(..., PLEASE_BE_MORE_LIKE_UNIX, ...).
On 11/20/19 3:40 PM, Thomas Munro wrote:
On Fri, Nov 8, 2019 at 3:41 AM Andrew Dunstan
<andrew.dunstan@2ndquadrant.com> wrote:On 11/7/19 9:12 AM, Alvaro Herrera wrote:
The patch says:
+ require Win32::API;
+ Win32::API->import;Oh, you're right, it does. I wonder why, though:
On further inspection I think those lines are unnecessary. The remainder
of the patch doesn't use this at all, AFAICT.So does that mean we're back on, we can use a patch like Juan Jose's?
I'd love to get rid of these intermittent buildfarm failures, like
this one just now:https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=drongo&dt=2019-11-20%2010%3A00%3A10
Here you can see:
could not read "C:/prog/bf/root/HEAD/pgsql.build/src/bin/pg_ctl/tmp_check/t_004_logrotate_primary_data/pgdata/current_logfiles":
Permission denied at
C:/prog/bf/root/HEAD/pgsql.build/src/test/perl/TestLib.pm line 397.That line is in the subroutine slurp_file, and says open(my $in, '<',
$filename). Using various clues from this thread, it seems like we
could, on Windows only, add code to TestLib.pm's INIT to rebind
*CORE::GLOBAL::open to a wrapper function that would just do
CreateFile(..., PLEASE_BE_MORE_LIKE_UNIX, ...).
Possibly. I will do some testing on drongo in the next week or so.
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Thu, Nov 21, 2019 at 3:35 PM Andrew Dunstan <
andrew.dunstan@2ndquadrant.com> wrote:
On 11/20/19 3:40 PM, Thomas Munro wrote:
On Fri, Nov 8, 2019 at 3:41 AM Andrew Dunstan
<andrew.dunstan@2ndquadrant.com> wrote:On 11/7/19 9:12 AM, Alvaro Herrera wrote:
The patch says:
+ require Win32::API;
+ Win32::API->import;Oh, you're right, it does. I wonder why, though:
On further inspection I think those lines are unnecessary. The remainder
of the patch doesn't use this at all, AFAICT.So does that mean we're back on, we can use a patch like Juan Jose's?
I'd love to get rid of these intermittent buildfarm failures, like
this one just now:https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=drongo&dt=2019-11-20%2010%3A00%3A10
Here you can see:
could not read
"C:/prog/bf/root/HEAD/pgsql.build/src/bin/pg_ctl/tmp_check/t_004_logrotate_primary_data/pgdata/current_logfiles":
Permission denied at
C:/prog/bf/root/HEAD/pgsql.build/src/test/perl/TestLib.pm line 397.That line is in the subroutine slurp_file, and says open(my $in, '<',
$filename). Using various clues from this thread, it seems like we
could, on Windows only, add code to TestLib.pm's INIT to rebind
*CORE::GLOBAL::open to a wrapper function that would just do
CreateFile(..., PLEASE_BE_MORE_LIKE_UNIX, ...).Possibly. I will do some testing on drongo in the next week or so.
I think Perl's open() is a bad candidate for an overload, so I will update
the previous patch that only touches slurp_file().
This version address the issues with the required libraries and uses
functions that expose less of the Windows API.
Regards,
Juan José Santamaría Flecha
Attachments:
0001-tap-file_share-windows-file-access-v2.patchapplication/octet-stream; name=0001-tap-file_share-windows-file-access-v2.patchDownload
diff --git a/src/test/perl/TestLib.pm b/src/test/perl/TestLib.pm
index 905d0d1..20a38ff 100644
--- a/src/test/perl/TestLib.pm
+++ b/src/test/perl/TestLib.pm
@@ -112,6 +112,15 @@ BEGIN
# Must be set early
$windows_os = $Config{osname} eq 'MSWin32' || $Config{osname} eq 'msys';
+ if ($windows_os)
+ {
+ require Win32API::File;
+ Win32API::File->import(qw(
+ createFile
+ OsFHandleOpen
+ CloseHandle
+ ));
+ }
}
=pod
@@ -394,10 +403,24 @@ sub slurp_file
{
my ($filename) = @_;
local $/;
- open(my $in, '<', $filename)
- or die "could not read \"$filename\": $!";
- my $contents = <$in>;
- close $in;
+ my $contents;
+ if (!$windows_os)
+ {
+ open(my $in, '<', $filename)
+ or die "could not read \"$filename\": $!";
+ $contents = <$in>;
+ close $in;
+ }
+ else
+ {
+ my $fHandle = createFile($filename, "r", "rwd")
+ or die "could not open \"$filename\": $^E";
+ OsFHandleOpen(my $fh = IO::Handle->new(), $fHandle, 'r')
+ or die "could not read \"$filename\": $^E\n";
+ $contents = <$fh>;
+ CloseHandle($fHandle)
+ or die "could not close \"$filename\": $^E\n";
+ }
$contents =~ s/\r//g if $Config{osname} eq 'msys';
return $contents;
}
On Thu, Nov 21, 2019 at 08:09:38PM +0100, Juan José Santamaría Flecha wrote:
I think Perl's open() is a bad candidate for an overload, so I will update
the previous patch that only touches slurp_file().
FWIW, I don't like much the approach of patching only slurp_file().
What gives us the guarantee that we won't have this discussion again
in a couple of months or years once a new caller of open() is added
for some new TAP tests, and that it has the same problems with
multi-process concurrency?
--
Michael
On Fri, Nov 22, 2019 at 9:00 AM Michael Paquier <michael@paquier.xyz> wrote:
On Thu, Nov 21, 2019 at 08:09:38PM +0100, Juan José Santamaría Flecha
wrote:I think Perl's open() is a bad candidate for an overload, so I will
update
the previous patch that only touches slurp_file().
FWIW, I don't like much the approach of patching only slurp_file().
What gives us the guarantee that we won't have this discussion again
in a couple of months or years once a new caller of open() is added
for some new TAP tests, and that it has the same problems with
multi-process concurrency?
I agree on that, from a technical stand point, overloading open() is
probably the best solution for the reasons above mentioned. My doubts come
from the effort such a solution will take and its maintainability, also
taking into account that there are not that many calls to open() in
"src/test/perl".
Regards,
Juan José Santamaría Flecha
On 11/22/19 3:55 AM, Juan José Santamaría Flecha wrote:
On Fri, Nov 22, 2019 at 9:00 AM Michael Paquier <michael@paquier.xyz
<mailto:michael@paquier.xyz>> wrote:On Thu, Nov 21, 2019 at 08:09:38PM +0100, Juan José Santamaría
Flecha wrote:I think Perl's open() is a bad candidate for an overload, so I
will update
the previous patch that only touches slurp_file().
FWIW, I don't like much the approach of patching only slurp_file().
What gives us the guarantee that we won't have this discussion again
in a couple of months or years once a new caller of open() is added
for some new TAP tests, and that it has the same problems with
multi-process concurrency?I agree on that, from a technical stand point, overloading open() is
probably the best solution for the reasons above mentioned. My doubts
come from the effort such a solution will take and its
maintainability, also taking into account that there are not that many
calls to open() in "src/test/perl".
I think the best course is for us to give your latest patch an outing on
the buildfarm and verify that the issues seen with slurp_file disappear.
That shouldn't take us more than a week or two to see - drongo has had 6
such failures in the last 11 days on master. After that we can discuss
how much further we might want to take it.
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
I think the best course is for us to give your latest patch an outing on
the buildfarm and verify that the issues seen with slurp_file disappear.
That shouldn't take us more than a week or two to see - drongo has had 6
such failures in the last 11 days on master. After that we can discuss
how much further we might want to take it.
Sounds sensible to me. We don't yet have verification that this is
even where the problem is ...
regards, tom lane
On 11/22/19 3:46 PM, Tom Lane wrote:
Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
I think the best course is for us to give your latest patch an outing on
the buildfarm and verify that the issues seen with slurp_file disappear.
That shouldn't take us more than a week or two to see - drongo has had 6
such failures in the last 11 days on master. After that we can discuss
how much further we might want to take it.Sounds sensible to me. We don't yet have verification that this is
even where the problem is ...
Done.
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
On 11/22/19 3:46 PM, Tom Lane wrote:
Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
I think the best course is for us to give your latest patch an outing on
the buildfarm and verify that the issues seen with slurp_file disappear.
That shouldn't take us more than a week or two to see - drongo has had 6
such failures in the last 11 days on master. After that we can discuss
how much further we might want to take it.
Sounds sensible to me. We don't yet have verification that this is
even where the problem is ...
Done.
?? I don't see any commit ...
regards, tom lane
On 11/24/19 6:46 PM, Tom Lane wrote:
Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
On 11/22/19 3:46 PM, Tom Lane wrote:
Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
I think the best course is for us to give your latest patch an outing on
the buildfarm and verify that the issues seen with slurp_file disappear.
That shouldn't take us more than a week or two to see - drongo has had 6
such failures in the last 11 days on master. After that we can discuss
how much further we might want to take it.Sounds sensible to me. We don't yet have verification that this is
even where the problem is ...Done.
?? I don't see any commit ...
Yeash, forgot to push, sorry.
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hello hackers,
Are there any plans to backport the patch to earlier versions
of the Postgres?
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=114541d58e5970e51b78b77b65de16210beaab43
We rarely see the issue with the pg_ctl/004_logrotate test on
the REL_12_STABLE branch. On my notebook I can easily reproduce
the "Permission denied at src/test/perl/TestLib.pm line 259"
error with the small change below. But the same test on the
13th version and the 12th version with the TestLib patch does
not fail.
diff --git a/src/bin/pg_ctl/t/004_logrotate.pl
b/src/bin/pg_ctl/t/004_logrotate.pl index bc39abd23e4..e49e159bc84 100644
---
a/src/bin/pg_ctl/t/004_logrotate.pl +++
b/src/bin/pg_ctl/t/004_logrotate.pl @@ -72,7 +72,7 @@ for (my
$attempts = 0; $attempts < $max_attempts; $attempts++)
{
$new_current_logfiles = slurp_file($node->data_dir .
'/current_logfiles');
last
if $new_current_logfiles ne $current_logfiles;
- usleep(100_000);
+ usleep(1);
}
note "now current_logfiles = $new_current_logfiles";
On 2019-11-22 20:22, Andrew Dunstan wrote:
On 11/22/19 3:55 AM, Juan José Santamaría Flecha wrote:
On Fri, Nov 22, 2019 at 9:00 AM Michael Paquier <michael@paquier.xyz
<mailto:michael@paquier.xyz>> wrote:On Thu, Nov 21, 2019 at 08:09:38PM +0100, Juan José Santamaría
Flecha wrote:I think Perl's open() is a bad candidate for an overload, so I
will update
the previous patch that only touches slurp_file().
FWIW, I don't like much the approach of patching only
slurp_file().
What gives us the guarantee that we won't have this discussion
again
in a couple of months or years once a new caller of open() is
added
for some new TAP tests, and that it has the same problems with
multi-process concurrency?I agree on that, from a technical stand point, overloading open() is
probably the best solution for the reasons above mentioned. My doubts
come from the effort such a solution will take and its
maintainability, also taking into account that there are not that many
calls to open() in "src/test/perl".I think the best course is for us to give your latest patch an outing
on
the buildfarm and verify that the issues seen with slurp_file
disappear.
That shouldn't take us more than a week or two to see - drongo has had
6
such failures in the last 11 days on master. After that we can discuss
how much further we might want to take it.cheers
andrew
--
regards,
Roman Zharkov
On 12/15/20 12:05 AM, r.zharkov@postgrespro.ru wrote:
Hello hackers,
Are there any plans to backport the patch to earlier versions
of the Postgres?
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=114541d58e5970e51b78b77b65de16210beaab43We rarely see the issue with the pg_ctl/004_logrotate test on
the REL_12_STABLE branch. On my notebook I can easily reproduce
the "Permission denied at src/test/perl/TestLib.pm line 259"
error with the small change below. But the same test on the
13th version and the 12th version with the TestLib patch does
not fail.diff --git a/src/bin/pg_ctl/t/004_logrotate.pl b/src/bin/pg_ctl/t/004_logrotate.pl index bc39abd23e4..e49e159bc84 100644 --- a/src/bin/pg_ctl/t/004_logrotate.pl +++ b/src/bin/pg_ctl/t/004_logrotate.pl @@ -72,7 +72,7 @@ for (my $attempts = 0; $attempts < $max_attempts; $attempts++) { $new_current_logfiles = slurp_file($node->data_dir . '/current_logfiles'); last if $new_current_logfiles ne $current_logfiles; - usleep(100_000); + usleep(1); } note "now current_logfiles = $new_current_logfiles";
Oops, looks like that slipped off my radar somehow, I'll see about
backpatching it right away.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
Thank you very much!
On 2020-12-15 20:47, Andrew Dunstan wrote:
On 12/15/20 12:05 AM, r.zharkov@postgrespro.ru wrote:
Are there any plans to backport the patch to earlier versions
of the Postgres?
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=114541d58e5970e51b78b77b65de16210beaab43Oops, looks like that slipped off my radar somehow, I'll see about
backpatching it right away.cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
--
regards,
Roman Zharkov