Date: Sun, 2 Feb 2020 09:01:55 -0800
From: Noah Misch <noah@leadboat.com>
To: pgsql-hackers@postgresql.org
Subject: TestLib condition for deleting temporary directories
Message-ID: <20200202170155.GA3264196@rfd.leadboat.com>
References: 
 <CAJ3gD9dV9SBCoMOJDZPDrB9-W74UpMvEeL+DBqg2Yg2BSmQstQ@mail.gmail.com>
 <CAA4eK1JdK+-uz_fXOo+p6no7jxvthjmxDkqyqC2bmKvHXiRUjw@mail.gmail.com>
 <CAJ3gD9edQpM6XkNGaaQ1eN9698-DOiQbF8UoseVXAQPwT0Se7A@mail.gmail.com>
 <CAA4eK1JSEn=UuTiYiAwqAWWEmLvMtnPMQ2+my0X8diEgtG6LqA@mail.gmail.com>
 <CAA4eK1LMDx6vK8Kdw8WUeW1MjToN2xVffL2kvtHvZg17=Y6QQg@mail.gmail.com>
 <CAA4eK1LJqMuXoCLuxkTr1HidbR8DkgRrVC7jHWDyXT=FD2gt6Q@mail.gmail.com>
 <CAJ3gD9dFYJnOxUsR3MeJ=3hJKZk5XqAx-6fYUNXt6jJOK3QFMQ@mail.gmail.com>
 <20200104185148.GA2270238@rfd.leadboat.com>
 <CAJ3gD9diTB8rqyUWo=jZrSQxvW7o-gGaO6uoF5racneH3+FYOg@mail.gmail.com>
 <20200109053704.GA2502006@rfd.leadboat.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="AhhlLboLdkugWU4S"
Content-Disposition: inline
In-Reply-To: <20200109053704.GA2502006@rfd.leadboat.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Precedence: bulk


--AhhlLboLdkugWU4S
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

Forking thread "logical decoding : exceeded maxAllocatedDescs for .spill
files" for this side issue:

On Wed, Jan 08, 2020 at 09:37:04PM -0800, Noah Misch wrote:
> v10
> deletes PostgresNode base directories at the end of this test file, despite
> the failure[1].

> [1] It has the all_tests_passing() logic in an attempt to stop this.  I'm
> guessing it didn't help because the file failed by calling die "connection
> error: ...", not by reporting a failure to Test::More via ok(0) or similar.

That is what happened.  We should test the exit status to decide whether to
keep temporaries, as attached.  PostgresNode does that, since commit 90627cf
(thread https://postgr.es/m/flat/6205.1492883490%40sss.pgh.pa.us).  That
thread already discussed $SUBJECT[1] and the __DIE__ handler being
redundant[2].  I plan to back-patch, since it's most useful for v10 and v9.6.

[1] https://postgr.es/m/CAMsr+YFyFU=+MVFZqhthfMW22x5-h517e6ck6ET+DT+X4bUO7g@mail.gmail.com
[2] https://postgr.es/m/FEA925B2-C3AE-4BA9-9194-5F5616AD0794@yesql.se

--AhhlLboLdkugWU4S
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="perl-temporaries-v1.patch"

Author:     Noah Misch <noah@leadboat.com>
Commit:     Noah Misch <noah@leadboat.com>

    When a TAP file has non-zero exit status, retain temporary directories.
    
    PostgresNode already retained base directories in such cases.  Stop
    using $SIG{__DIE__}, which is redundant with the exit status check, in
    lieu of proliferating it to TestLib.  Back-patch to 9.6, where commit
    88802e068017bee8cea7a5502a712794e761c7b5 introduced retention on
    failure.
    
    Reviewed by FIXME.
    
    Discussion: https://postgr.es/m/FIXME

diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm
index 2e0cf4a..ff972d1 100644
--- a/src/test/perl/PostgresNode.pm
+++ b/src/test/perl/PostgresNode.pm
@@ -1206,20 +1206,6 @@ sub can_bind
 	return $ret;
 }
 
-# Retain the errno on die() if set, else assume a generic errno of 1.
-# This will instruct the END handler on how to handle artifacts left
-# behind from tests.
-$SIG{__DIE__} = sub {
-	if ($!)
-	{
-		$died = $!;
-	}
-	else
-	{
-		$died = 1;
-	}
-};
-
 # Automatically shut down any still-running nodes when the test script exits.
 # Note that this just stops the postmasters (in the same order the nodes were
 # created in).  Any temporary directories are deleted, in an unspecified
@@ -1238,8 +1224,7 @@ END
 		next if defined $ENV{'PG_TEST_NOCLEAN'};
 
 		# clean basedir on clean test invocation
-		$node->clean_node
-		  if TestLib::all_tests_passing() && !defined $died && !$exit_code;
+		$node->clean_node if $exit_code == 0 && TestLib::all_tests_passing();
 	}
 
 	$? = $exit_code;
diff --git a/src/test/perl/TestLib.pm b/src/test/perl/TestLib.pm
index 458b801..65ee042 100644
--- a/src/test/perl/TestLib.pm
+++ b/src/test/perl/TestLib.pm
@@ -183,8 +183,13 @@ INIT
 END
 {
 
-	# Preserve temporary directory for this test on failure
-	$File::Temp::KEEP_ALL = 1 unless all_tests_passing();
+	# Test files have several ways of causing prove_check to fail:
+	# 1. Exit with a non-zero status.
+	# 2. Call ok(0) or similar, indicating that a constituent test failed.
+	# 3. Deviate from the planned number of tests.
+	#
+	# Preserve temporary directories after (1) and after (2).
+	$File::Temp::KEEP_ALL = 1 unless $? == 0 && all_tests_passing();
 }
 
 =pod

--AhhlLboLdkugWU4S--