Thread: 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":
Permissiondenied 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
Attachment
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
Re: TAP tests aren't using the magic words for Windows file access
From
Juan José Santamaría Flecha
Date:
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::File
Compatibility-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
Attachment
=?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
Attachment
Re: TAP tests aren't using the magic words for Windows file access
From
Juan José Santamaría Flecha
Date:
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
Re: TAP tests aren't using the magic words for Windows file access
From
Juan José Santamaría Flecha
Date:
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
Attachment
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
Attachment
Re: TAP tests aren't using the magic words for Windows file access
From
Juan José Santamaría Flecha
Date:
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
Re: TAP tests aren't using the magic words for Windows file access
From
r.zharkov@postgrespro.ru
Date:
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=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"; > > > 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
Re: TAP tests aren't using the magic words for Windows file access
From
r.zharkov@postgrespro.ru
Date:
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=114541d58e5970e51b78b77b65de16210beaab43 > > > 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 -- regards, Roman Zharkov