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+35-4
=?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+27-4
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
