PATCH: Unlogged tables re-initialization tests

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
17 messages Options
Reply | Threaded
Open this post in threaded view
|

PATCH: Unlogged tables re-initialization tests

David Steele
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
[hidden email]

[1]https://www.postgresql.org/message-id/04791bab-cb04-ba43-e9c0-664a4c1ffb2c%40pgmasters.net

recovery-reinit-unlogged-test.patch (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: PATCH: Unlogged tables re-initialization tests

Thomas Munro-3
On Thu, Mar 1, 2018 at 9:24 AM, David Steele <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: PATCH: Unlogged tables re-initialization tests

David Steele
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 <[hidden email]> 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
[hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: PATCH: Unlogged tables re-initialization tests

Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: PATCH: Unlogged tables re-initialization tests

David Steele
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
[hidden email]

recovery-reinit-unlogged-test-v2.patch (4K) Download Attachment
signature.asc (891 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: PATCH: Unlogged tables re-initialization tests

Peter Eisentraut-6
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

Reply | Threaded
Open this post in threaded view
|

Re: PATCH: Unlogged tables re-initialization tests

Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: PATCH: Unlogged tables re-initialization tests

Peter Eisentraut-6
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

Reply | Threaded
Open this post in threaded view
|

Re: PATCH: Unlogged tables re-initialization tests

David Steele
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
[hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: PATCH: Unlogged tables re-initialization tests

Dagfinn Ilmari Mannsåker
David Steele <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: PATCH: Unlogged tables re-initialization tests

David Steele
On 3/12/18 11:59 AM, Dagfinn Ilmari Mannsåker wrote:

> David Steele <[hidden email]> 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
[hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: PATCH: Unlogged tables re-initialization tests

Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: PATCH: Unlogged tables re-initialization tests

David Steele
In reply to this post by Peter Eisentraut-6
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
[hidden email]

recovery-reinit-unlogged-test-v3.patch (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: PATCH: Unlogged tables re-initialization tests

Álvaro Herrera
In reply to this post by Dagfinn Ilmari Mannsåker
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

Reply | Threaded
Open this post in threaded view
|

Re: PATCH: Unlogged tables re-initialization tests

Peter Eisentraut-6
In reply to this post by David Steele
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

Reply | Threaded
Open this post in threaded view
|

Re: PATCH: Unlogged tables re-initialization tests

David Steele
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
[hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: PATCH: Unlogged tables re-initialization tests

Peter Eisentraut-6
In reply to this post by Álvaro Herrera
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

Previous Thread Next Thread