Thread: PATCH: Unlogged tables re-initialization tests
These tests were originally included in the exclude unlogged tables patch [1] to provide coverage for the refactoring of reinit.c. After review we found a simpler implementation that did not require the reinit.c refactor so I dropped the tests from that patch. I did not include the refactor here because it's just noise now, but I think the tests still have value. I will add to the 2018-03 CF. -- -David david@pgmasters.net [1]https://www.postgresql.org/message-id/04791bab-cb04-ba43-e9c0-664a4c1ffb2c%40pgmasters.net
Attachment
On Thu, Mar 1, 2018 at 9:24 AM, David Steele <david@pgmasters.net> wrote: > These tests were originally included in the exclude unlogged tables > patch [1] to provide coverage for the refactoring of reinit.c. Hi David, +# The following tests test symlinks. Windows doesn't have symlinks, so +# skip on Windows. Could you please explain this a bit more? I don't see any symlinks being used directly in this test. I do see CREATE TABLESPACE and that uses symlink(), but win32_port.h converts that to "junctions", apparently the Windows equivalent. Is there some reason that doesn't work with this test? If, by any chance, you are talking about whatever dark force prevents the "tablespace" regression test from succeeding when run as a user with administrative privileges on Windows, then I would *love* to hear an explanation for that phenomenon and how to fix it, because it currently prevents me from automatically testing all Commitfest patches on the appveyor.com Windows build farm. I know that it passes as a non-administrative user. -- Thomas Munro http://www.enterprisedb.com
Hi Thomas, [Also pulling in Michael for Windows knowledge] On 3/1/18 12:27 AM, Thomas Munro wrote: > On Thu, Mar 1, 2018 at 9:24 AM, David Steele <david@pgmasters.net> wrote: >> These tests were originally included in the exclude unlogged tables >> patch [1] to provide coverage for the refactoring of reinit.c. > > +# The following tests test symlinks. Windows doesn't have symlinks, so > +# skip on Windows. > > Could you please explain this a bit more? I don't see any symlinks > being used directly in this test. I do see CREATE TABLESPACE and that > uses symlink(), but win32_port.h converts that to "junctions", > apparently the Windows equivalent. Is there some reason that doesn't > work with this test? I copied this pattern from src/bin/pg_basebackup/t/010_pg_basebackup.pl which indicates that Windows will not support these types of tests. But your point is well-taken. No symlinks are used in this test so it *should* work. Michael, what do you think? > If, by any chance, you are talking about whatever dark force prevents > the "tablespace" regression test from succeeding when run as a user > with administrative privileges on Windows, then I would *love* to hear > an explanation for that phenomenon and how to fix it, because it > currently prevents me from automatically testing all Commitfest > patches on the appveyor.com Windows build farm. I know that it passes > as a non-administrative user. Sorry, no! -- -David david@pgmasters.net
On Thu, Mar 01, 2018 at 01:12:19PM -0500, David Steele wrote: > But your point is well-taken. No symlinks are used in this test so it > *should* work. > > Michael, what do you think? Perl's symlink() does not work on Windows. It does not fly higher than that, and that's the reason why a good chunk of the tests are skipped for pg_basebackup. If perl was to have a version of symlink() which works, say with junction points, or if Windows was to have a sane symlink implementation (or with [1]?), or if it was possible to create junction points using an in-core implementation in perl, then those tests could not be skipped. But it seems that none of those scenarios have happened yet. From what I read in your patch, it seems to me that this test should work. If they don't for whatever reason, your patch then does not give a correct justification explaining why they should be skipped. [1]: https://blogs.windows.com/buildingapps/2016/12/02/symlinks-windows-10/ -- Michael
Attachment
Hi, On 3/1/18 11:59 PM, Michael Paquier wrote: > On Thu, Mar 01, 2018 at 01:12:19PM -0500, David Steele wrote: >> But your point is well-taken. No symlinks are used in this test so it >> *should* work. >> >> Michael, what do you think? > > Perl's symlink() does not work on Windows. It does not fly higher than > that, and that's the reason why a good chunk of the tests are skipped > for pg_basebackup. If perl was to have a version of symlink() which > works, say with junction points, or if Windows was to have a sane > symlink implementation (or with [1]?), or if it was possible to create > junction points using an in-core implementation in perl, then those > tests could not be skipped. But it seems that none of those scenarios > have happened yet. > > From what I read in your patch, it seems to me that this test should > work. If they don't for whatever reason, your patch then does not give > a correct justification explaining why they should be skipped. Thanks for the input, Michael. Attached is a new version that does not skip tests on Windows. I don't have any way to test it, but if one of you do that would be much appreciated. Thanks! -- -David david@pgmasters.net
Attachment
This seems like a useful test.
On 3/5/18 12:35, David Steele wrote:
> +mkdir($tablespaceDir)
> + or die "unable to mkdir \"$tablespaceDir\"";
Use BAIL_OUT instead of die in tests.
> + $ts1UnloggedPath = $node->safe_psql('postgres',
> + q{select pg_relation_filepath('ts1_unlogged')});
strange indentation
> +# Write forks to test that they are removed during recovery
> +$node->command_ok(['touch', "$pgdata/${baseUnloggedPath}_vm"],
> + 'touch vm fork in base');
> +$node->command_ok(['touch', "$pgdata/${baseUnloggedPath}_fsm"],
> + 'touch fsm fork in base');
These are not tests, just some prep work. So they should not use
command_ok.
It would probably also be better to avoid the Unix-specific touch
command and instead use Perl code to open and write to the files.
> +# Check unlogged table in base
> +ok(-f "$pgdata/${baseUnloggedPath}_init", 'init fork in base');
> +ok(-f "$pgdata/$baseUnloggedPath", 'main fork in base');
> +ok(!-f "$pgdata/${baseUnloggedPath}_vm", 'vm fork not in base');
> +ok(!-f "$pgdata/${baseUnloggedPath}_fsm", 'fsm fork not in base');
These test names could be a bit more verbose and distinct, for example,
'main fork was recreated after restart'.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Mar 09, 2018 at 05:23:48PM -0500, Peter Eisentraut wrote: > This seems like a useful test. > > On 3/5/18 12:35, David Steele wrote: > > +mkdir($tablespaceDir) > > + or die "unable to mkdir \"$tablespaceDir\""; > > Use BAIL_OUT instead of die in tests. Would it be better to make this practice more uniform? From the code of the tests: $ git grep die -- */t/*.pl | wc -l 50 -- Michael
Attachment
On 3/11/18 05:11, Michael Paquier wrote: > On Fri, Mar 09, 2018 at 05:23:48PM -0500, Peter Eisentraut wrote: >> This seems like a useful test. >> >> On 3/5/18 12:35, David Steele wrote: >>> +mkdir($tablespaceDir) >>> + or die "unable to mkdir \"$tablespaceDir\""; >> >> Use BAIL_OUT instead of die in tests. > > Would it be better to make this practice more uniform? From the code of > the tests: > $ git grep die -- */t/*.pl | wc -l > 50 Yes, or maybe there is a way to "catch" die and turn it into BAIL_OUT? -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 3/12/18 11:27 AM, Peter Eisentraut wrote:
> On 3/11/18 05:11, Michael Paquier wrote:
>> On Fri, Mar 09, 2018 at 05:23:48PM -0500, Peter Eisentraut wrote:
>>> This seems like a useful test.
>>>
>>> On 3/5/18 12:35, David Steele wrote:
>>>> +mkdir($tablespaceDir)
>>>> + or die "unable to mkdir \"$tablespaceDir\"";
>>>
>>> Use BAIL_OUT instead of die in tests.
>>
>> Would it be better to make this practice more uniform? From the code of
>> the tests:
>> $ git grep die -- */t/*.pl | wc -l
>> 50
>
> Yes, or maybe there is a way to "catch" die and turn it into BAIL_OUT?
something like this should work:
# Convert die to BAIL_OUT
$SIG{__DIE__} = sub {BAIL_OUT(@_)};
--
-David
david@pgmasters.net
Re: PATCH: Unlogged tables re-initialization tests
From
ilmari@ilmari.org (Dagfinn Ilmari Mannsåker)
Date:
David Steele <david@pgmasters.net> writes:
> On 3/12/18 11:27 AM, Peter Eisentraut wrote:
>> On 3/11/18 05:11, Michael Paquier wrote:
>>> On Fri, Mar 09, 2018 at 05:23:48PM -0500, Peter Eisentraut wrote:
>>>> This seems like a useful test.
>>>>
>>>> On 3/5/18 12:35, David Steele wrote:
>>>>> +mkdir($tablespaceDir)
>>>>> + or die "unable to mkdir \"$tablespaceDir\"";
>>>>
>>>> Use BAIL_OUT instead of die in tests.
>>>
>>> Would it be better to make this practice more uniform? From the code of
>>> the tests:
>>> $ git grep die -- */t/*.pl | wc -l
>>> 50
>>
>> Yes, or maybe there is a way to "catch" die and turn it into BAIL_OUT?
>
> something like this should work:
>
> # Convert die to BAIL_OUT
> $SIG{__DIE__} = sub {BAIL_OUT(@_)};
$SIG{__DIE__} gets called even for exceptions that would be caught by a
surrunding eval block, so this should at the very least be:
$SIG{__DIE__} = sub { BAIL_OUT(@_) unless $^S };
However, that is still wrong, because die() and BAIL_OUT() mean
different things: die() aborts the current test script, while BAIL_OUT()
aborts the entire 'prove' run, i.e. all subsequent scripts in the same
test directory.
- ilmari
--
- Twitter seems more influential [than blogs] in the 'gets reported in
the mainstream press' sense at least. - Matt McLeod
- That'd be because the content of a tweet is easier to condense down
to a mainstream media article. - Calle Dybedahl
On 3/12/18 11:59 AM, Dagfinn Ilmari Mannsåker wrote:
> David Steele <david@pgmasters.net> writes:
>
>> On 3/12/18 11:27 AM, Peter Eisentraut wrote:
>>> On 3/11/18 05:11, Michael Paquier wrote:
>>>> On Fri, Mar 09, 2018 at 05:23:48PM -0500, Peter Eisentraut wrote:
>>>>> This seems like a useful test.
>>>>>
>>>>> On 3/5/18 12:35, David Steele wrote:
>>>>>> +mkdir($tablespaceDir)
>>>>>> + or die "unable to mkdir \"$tablespaceDir\"";
>>>>>
>>>>> Use BAIL_OUT instead of die in tests.
>>>>
>>>> Would it be better to make this practice more uniform? From the code of
>>>> the tests:
>>>> $ git grep die -- */t/*.pl | wc -l
>>>> 50
>>>
>>> Yes, or maybe there is a way to "catch" die and turn it into BAIL_OUT?
>>
>> something like this should work:
>>
>> # Convert die to BAIL_OUT
>> $SIG{__DIE__} = sub {BAIL_OUT(@_)};
>
> $SIG{__DIE__} gets called even for exceptions that would be caught by a
> surrunding eval block, so this should at the very least be:
>
> $SIG{__DIE__} = sub { BAIL_OUT(@_) unless $^S };
>
> However, that is still wrong, because die() and BAIL_OUT() mean
> different things: die() aborts the current test script, while BAIL_OUT()
> aborts the entire 'prove' run, i.e. all subsequent scripts in the same
> test directory.
If that's the case, do we really want to abort all subsequent test
modules if a single module fails? I'm good either way, just throwing it
out there for consideration.
--
-David
david@pgmasters.net
On Mon, Mar 12, 2018 at 02:33:18PM -0400, David Steele wrote: > On 3/12/18 11:59 AM, Dagfinn Ilmari Mannsåker wrote: >> However, that is still wrong, because die() and BAIL_OUT() mean >> different things: die() aborts the current test script, while BAIL_OUT() >> aborts the entire 'prove' run, i.e. all subsequent scripts in the same >> test directory. > > If that's the case, do we really want to abort all subsequent test > modules if a single module fails? I'm good either way, just throwing it > out there for consideration. I am getting that in those code paths, we want the test series to die in a painful way which should be used for tests where things are critically failing. In which case, as documented by Test:More we should use BAIL_OUT. It is true that using die() has the advantage that one can look at multiple failures in a single run when you want to look for similar failure patterns, however it makes debugging harder if you a lot of test scripts because it becomes harder to track which one was the first to fail in a parallel test. Also, if you look at all the code paths calling die() in the test suites those refer to critical failures, which should cause an immediate stop of the test. So my take would be to switch all die() calls to BAIL_OUT() in order to make all this code consistent. -- Michael
Attachment
Thanks for reviewing, Peter.
On 3/9/18 5:23 PM, Peter Eisentraut wrote:
> This seems like a useful test.
>
> On 3/5/18 12:35, David Steele wrote:
>> +mkdir($tablespaceDir)
>> + or die "unable to mkdir \"$tablespaceDir\"";
>
> Use BAIL_OUT instead of die in tests.
Done.
>> + $ts1UnloggedPath = $node->safe_psql('postgres',
>> + q{select pg_relation_filepath('ts1_unlogged')});
>
> strange indentation
Sure is. Fixed.
>> +# Write forks to test that they are removed during recovery
>> +$node->command_ok(['touch', "$pgdata/${baseUnloggedPath}_vm"],
>> + 'touch vm fork in base');
>> +$node->command_ok(['touch', "$pgdata/${baseUnloggedPath}_fsm"],
>> + 'touch fsm fork in base');
>
> These are not tests, just some prep work. So they should not use
> command_ok.
Removed command_ok().
> It would probably also be better to avoid the Unix-specific touch
> command and instead use Perl code to open and write to the files.
Updated to use append_to_file().
>> +# Check unlogged table in base
>> +ok(-f "$pgdata/${baseUnloggedPath}_init", 'init fork in base');
>> +ok(-f "$pgdata/$baseUnloggedPath", 'main fork in base');
>> +ok(!-f "$pgdata/${baseUnloggedPath}_vm", 'vm fork not in base');
>> +ok(!-f "$pgdata/${baseUnloggedPath}_fsm", 'fsm fork not in base');
>
> These test names could be a bit more verbose and distinct, for example,
> 'main fork was recreated after restart'.
Done.
A new patch is attached.
Thanks,
--
-David
david@pgmasters.net
Attachment
Dagfinn Ilmari Mannsåker wrote:
> $SIG{__DIE__} gets called even for exceptions that would be caught by a
> surrunding eval block, so this should at the very least be:
>
> $SIG{__DIE__} = sub { BAIL_OUT(@_) unless $^S };
>
> However, that is still wrong, because die() and BAIL_OUT() mean
> different things: die() aborts the current test script, while BAIL_OUT()
> aborts the entire 'prove' run, i.e. all subsequent scripts in the same
> test directory.
Sounds like 'die' is the correct thing, then, and that BAIL_OUT should
be called sparingly ... for example this one in PostgresNode::start
seems like an overreaction:
BAIL_OUT("node \"$name\" is already running") if defined $self->{_pid};
Yes?
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 3/13/18 10:12, David Steele wrote: > A new patch is attached. Committed. I removed the cleanup code at the end of the test file, since that is done automatically. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 3/14/18 9:07 AM, Peter Eisentraut wrote: > On 3/13/18 10:12, David Steele wrote: >> A new patch is attached. > > Committed. Thanks, Peter! -- -David david@pgmasters.net
On 3/13/18 12:46, Alvaro Herrera wrote:
> Dagfinn Ilmari Mannsåker wrote:
>
>> $SIG{__DIE__} gets called even for exceptions that would be caught by a
>> surrunding eval block, so this should at the very least be:
>>
>> $SIG{__DIE__} = sub { BAIL_OUT(@_) unless $^S };
>>
>> However, that is still wrong, because die() and BAIL_OUT() mean
>> different things: die() aborts the current test script, while BAIL_OUT()
>> aborts the entire 'prove' run, i.e. all subsequent scripts in the same
>> test directory.
>
> Sounds like 'die' is the correct thing, then, and that BAIL_OUT should
> be called sparingly ... for example this one in PostgresNode::start
> seems like an overreaction:
> BAIL_OUT("node \"$name\" is already running") if defined $self->{_pid};
It does sound like BAIL_OUT should be used more sparingly. However, if
you use die, then you don't get a good error message printed, just
something like
t/020_pg_receivewal.pl ... 9/18 # Looks like you planned 18 tests but
ran 12.
# Looks like your test exited with 25 just after 12.
t/020_pg_receivewal.pl ... Dubious, test returned 25 (wstat 6400, 0x1900)
Failed 6/18 subtests
whereas with BAIL_OUT you get
t/020_pg_receivewal.pl ... 9/18 Bailout called. Further testing
stopped: foobar
FAILED--Further testing stopped: foobar
The functional difference is significant however: BAIL_OUT prevents the
030 test file to be called, die does not.
Could use more research into best practices ....
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services