PATCH: Unlogged tables re-initialization tests

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]https://blogs.windows.com/buildingapps/2016/12/02/symlinks-windows-10/ -- Michael?), 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
--
Michael

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

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

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

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

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

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