From: Mark Dilger <mark.dilger@enterprisedb.com>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Peter Geoghegan <pg@bowt.ie>
Cc: Andrey M. Borodin <x4mmm@yandex-team.ru>
Cc: Stephen Frost <sfrost@snowman.net>
Cc: Michael Paquier <michael@paquier.xyz>
Cc: Amul Sul <sulamul@gmail.com>
Cc: Dilip Kumar <dilipbalaut@gmail.com>
Cc: Andres Freund <andres@anarazel.de>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: new heapcheck contrib module
Date: Thu, 22 Oct 2020 12:15:53 -0700
Message-ID: <2A7DA1A8-C4AA-43DF-A985-3CA52F4DC775@enterprisedb.com> (raw)
In-Reply-To: <721BFA83-EA6F-463D-A9DF-CE78A318B73A@enterprisedb.com>
References: <12ED3DA8-25F0-4B68-937D-D907CFBF08E7@enterprisedb.com>
<20200420194210.sevnpi4lyghoanxi@alap3.anarazel.de>
<23E60C3C-C20A-4508-84AC-E0CEA59E784A@enterprisedb.com>
<CA+TgmobAm8vB1MNQ1taO1et+HxTN8ULnQvEG+grNDqNQTBjbtA@mail.gmail.com>
<F845AC50-A479-43F3-ADFB-861A4D2AD287@enterprisedb.com>
<5F743835-3399-419C-8324-2D424237E999@enterprisedb.com>
<CAFiTN-syyHc3jZoou51v0SR8z0POoNfktqEO6MaGig4YS8mosA@mail.gmail.com>
<E6F4258A-B661-4F2C-8A35-278C9436930A@enterprisedb.com>
<CAFiTN-s-VX3DLfqv7OOo0zGCTOZ3xVU9UZ6GgzBZdJTPNnYFNg@mail.gmail.com>
<885D2EC9-7D06-4903-A17D-72C184A1E1C5@enterprisedb.com>
<CAFiTN-uYg_PiZ854=5R-D9skratapAHKhaDAG7fhJv-Vn5UhaQ@mail.gmail.com>
<5CC85F28-F210-47AC-8DA8-EC4E82AF3FD5@enterprisedb.com>
<CAFiTN-sP5iFJGERTq4a4OLTCzjZKhLC5P6_k2bxQKXnuvdmzGA@mail.gmail.com>
<CAFiTN-tTg0R5AHvXC1ro9XphroX1wUtc5t91hU_+7DeGTpE5VQ@mail.gmail.com>
<0C0349EB-DD85-4DA8-82C7-DF70AB714592@enterprisedb.com>
<BA6E870F-241C-4DDB-B92D-DB39E679C7D4@enterprisedb.com>
<CA+TgmobQUdfM2Q7WAR606RR2YehCZ5GHRKBc-VWfRj-1ZUKqDA@mail.gmail.com>
<C4A3E5AE-CBB2-4E3F-8BAE-29EE216762C2@enterprisedb.com>
<CAAJ_b96_Xr2a+D0j8xKbOnCzURcVQm0V2DS+T_pfc+wt3BpL8g@mail.gmail.com>
<9DF93B83-FE46-4EC4-A99D-77A60729FD2D@enterprisedb.com>
<CA+TgmoZT3kCOaKq6=m5wHGKh_9XQDbCC1LZXL6w_+NMB-n1+mw@mail.gmail.com>
<CAAJ_b94hsrF4x7UKQj03416fAEXJorUpZ9WhsZZjqgv-4+Wg9A@mail.gmail.com>
<50F6468F-5A5E-412A-86EC-DF24B549D733@enterprisedb.com>
<CAAJ_b94+A1Gk5r5sBYbYM2K5hH_gyp_o=+gwJuJhn5ZEm5_rqw@mail.gmail.com>
<CAAJ_b94Wh_7kM9JuJk7wnNsYSNPdSzz35cOEaE_ZRnPcqvupCg@mail.gmail.com>
<7D603B53-8078-4AF0-851B-803FF4416501@enterprisedb.com>
<CA+TgmoZbPtTgkQBGoco39uqKUWuv=0+TOkiNcCuqWWvUkJVuxQ@mail.gmail.com>
<A4C9E0AE-46BA-457F-A445-4527E5AA5E3C@enterprisedb.com>
<98265AA9-7BDC-4708-9348-CC1B173BBE1D@enterprisedb.com>
<CA+Tgmoam+pnGKhWS+RAYCk1eqjhbAPX7Vm69g1VF56xwyxXw+g@mail.gmail.com>
<209783.1603313248@sss.pgh.pa.us>
<77A60E99-6054-40FD-A5D6-769C287D4A0F@enterprisedb.com>
<CA+TgmoaD5K6UhO2zddxVCDY2TcR9wSLVurhQYAYpQ0VZPec0og@mail.gmail.com>
<CA+TgmobGxdm8Tsxawt7LW-qigHRHxD+b17PC+ft9EE+6pMhC9w@mail.gmail.com>
<721BFA83-EA6F-463D-A9DF-CE78A318B73A@enterprisedb.com>
> On Oct 22, 2020, at 9:01 AM, Mark Dilger <mark.dilger@enterprisedb.com> wrote:
>
>
>
>> On Oct 22, 2020, at 7:06 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>>
>> On Thu, Oct 22, 2020 at 8:51 AM Robert Haas <robertmhaas@gmail.com> wrote:
>>> Committed. Let's see what the buildfarm thinks.
>>
>> It is mostly happy, but thorntail is not:
>>
>> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=thorntail&dt=2020-10-22%2012%3A58%3A11
>>
>> I thought that the problem might be related to the fact that thorntail
>> is using force_parallel_mode, but I tried that here and it did not
>> cause a failure. So my next guess is that it is related to the fact
>> that this is a sparc64 machine, but it's hard to tell, since none of
>> the other sparc64 critters have run yet. In any case I don't know why
>> that would cause a failure. The messages in the log aren't very
>> illuminating, unfortunately. :-(
>>
>> Mark, any ideas what might cause specifically that set of tests to fail?
>
> The code is correctly handling an uncorrupted table, but then more or less randomly failing some of the time when processing a corrupt table.
>
> Tom identified a problem with an uninitialized variable. I'm putting together a new patch set to address it.
The 0001 attached patch addresses the -Werror=maybe-uninitialized problem.
The 0002 attached patch addresses the test failures:
The failing test is designed to stop the server, create blunt force trauma to the heap and toast files through overwriting garbage bytes, restart the server, and verify that corruption is detected by amcheck's verify_heapam(). The exact trauma is intended to be the same on all platforms, in terms of the number of bytes written and the location in the file that it gets written, but owing to differences between platforms, by design the test does not expect a particular corruption message.
The test was overwriting far fewer bytes than I had intended, but since it was still sufficient to create corruption on the platforms where I tested, I failed to notice. It should do a more thorough job now.
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachments:download all (2 files, .tar.gz)v21-0001-Fixing-unitialized-variable-bug.patch (application/octet-stream, 3.8K) download
From 967b1671a460affd54e0a67d5b707b79feca9f53 Mon Sep 17 00:00:00 2001
From: Mark Dilger <mark.dilger@enterprisedb.com>
Date: Thu, 22 Oct 2020 08:53:11 -0700
Subject: [PATCH v21 1/2] Fixing unitialized variable bug.
---
contrib/amcheck/verify_heapam.c | 65 +++++++++++++++------------------
1 file changed, 30 insertions(+), 35 deletions(-)
diff --git a/contrib/amcheck/verify_heapam.c b/contrib/amcheck/verify_heapam.cindex 0156c1e74a..2b0a0dec1d 100644--- a/contrib/amcheck/verify_heapam.c+++ b/contrib/amcheck/verify_heapam.c@@ -1368,60 +1368,55 @@ check_mxid_valid_in_rel(MultiXactId mxid, HeapCheckContext *ctx)
* truly been valid at that time.
*
* If the status argument is not NULL, and if and only if the transaction ID
- * appears to be valid in this relation, clog will be consulted and the commit- * status argument will be set with the status of the transaction ID.+ * appears to be valid in this relation, the status argument will be set with+ * the commit status of the transaction ID.
*/
static XidBoundsViolation
get_xid_status(TransactionId xid, HeapCheckContext *ctx,
XidCommitStatus *status)
{
- XidBoundsViolation result;
FullTransactionId fxid;
FullTransactionId clog_horizon;
/* Quick check for special xids */
if (!TransactionIdIsValid(xid))
- result = XID_INVALID;+ return XID_INVALID;
else if (xid == BootstrapTransactionId || xid == FrozenTransactionId)
- result = XID_BOUNDS_OK;- else
{
- /* Check if the xid is within bounds */- fxid = FullTransactionIdFromXidAndCtx(xid, ctx);- if (!fxid_in_cached_range(fxid, ctx))- {- /*- * We may have been checking against stale values. Update the- * cached range to be sure, and since we relied on the cached- * range when we performed the full xid conversion, reconvert.- */- update_cached_xid_range(ctx);- fxid = FullTransactionIdFromXidAndCtx(xid, ctx);- }+ if (status != NULL)+ *status = XID_COMMITTED;+ return XID_BOUNDS_OK;+ }- if (FullTransactionIdPrecedesOrEquals(ctx->next_fxid, fxid))- result = XID_IN_FUTURE;- else if (FullTransactionIdPrecedes(fxid, ctx->oldest_fxid))- result = XID_PRECEDES_CLUSTERMIN;- else if (FullTransactionIdPrecedes(fxid, ctx->relfrozenfxid))- result = XID_PRECEDES_RELMIN;- else- result = XID_BOUNDS_OK;+ /* Check if the xid is within bounds */+ fxid = FullTransactionIdFromXidAndCtx(xid, ctx);+ if (!fxid_in_cached_range(fxid, ctx))+ {+ /*+ * We may have been checking against stale values. Update the+ * cached range to be sure, and since we relied on the cached+ * range when we performed the full xid conversion, reconvert.+ */+ update_cached_xid_range(ctx);+ fxid = FullTransactionIdFromXidAndCtx(xid, ctx);
}
- /*- * Early return if the caller does not request clog checking, or if the- * xid is already known to be out of bounds. We dare not check clog for- * out of bounds transaction IDs.- */- if (status == NULL || result != XID_BOUNDS_OK)- return result;+ if (FullTransactionIdPrecedesOrEquals(ctx->next_fxid, fxid))+ return XID_IN_FUTURE;+ if (FullTransactionIdPrecedes(fxid, ctx->oldest_fxid))+ return XID_PRECEDES_CLUSTERMIN;+ if (FullTransactionIdPrecedes(fxid, ctx->relfrozenfxid))+ return XID_PRECEDES_RELMIN;++ /* Early return if the caller does not request clog checking */+ if (status == NULL)+ return XID_BOUNDS_OK;;
/* Early return if we just checked this xid in a prior call */
if (xid == ctx->cached_xid)
{
*status = ctx->cached_status;
- return result;+ return XID_BOUNDS_OK;
}
*status = XID_COMMITTED;
@@ -1443,5 +1438,5 @@ get_xid_status(TransactionId xid, HeapCheckContext *ctx,
LWLockRelease(XactTruncationLock);
ctx->cached_xid = xid;
ctx->cached_status = *status;
- return result;+ return XID_BOUNDS_OK;
}
--
2.21.1 (Apple Git-122.3)
=
v21-0002-Fixing-sloppy-regression-test-coding.patch (application/octet-stream, 2.0K) download
From 0413d6063ab04f3599dba1c423565a19dfd8b766 Mon Sep 17 00:00:00 2001
From: Mark Dilger <mark.dilger@enterprisedb.com>
Date: Thu, 22 Oct 2020 11:29:23 -0700
Subject: [PATCH v21 2/2] Fixing sloppy regression test coding
---
contrib/amcheck/t/001_verify_heapam.pl | 27 +++++++++++++++++++++-----
1 file changed, 22 insertions(+), 5 deletions(-)
diff --git a/contrib/amcheck/t/001_verify_heapam.pl b/contrib/amcheck/t/001_verify_heapam.plindex e7526c17b8..9b9190db35 100644--- a/contrib/amcheck/t/001_verify_heapam.pl+++ b/contrib/amcheck/t/001_verify_heapam.pl@@ -152,6 +152,20 @@ sub fresh_test_table
));
}
+sub syswrite_or_bail+{+ my ($fh, $contents, $length) = @_;++ my $written = syswrite($fh, $contents, $length);++ BAIL_OUT("syswrite failed: $!")+ unless defined $written;+ BAIL_OUT("short syswrite: $written bytes written, $length expected")+ if ($written < $length);+ BAIL_OUT("long syswrite: $written bytes written, $length expected")+ if ($written > $length);+}+
# Stops the test node, corrupts the first page of the named relation, and
# restarts the node.
sub corrupt_first_page_internal
@@ -161,19 +175,22 @@ sub corrupt_first_page_internal
$node->stop;
my $fh;
- open($fh, '+<', $relpath);+ open($fh, '+<', $relpath)+ or BAIL_OUT("open failed: $!");
binmode $fh;
# If we corrupt the header, postgres won't allow the page into the buffer.
- syswrite($fh, '\xFF\xFF\xFF\xFF', 8) if ($corrupt_header);+ syswrite_or_bail($fh, '\xFF' x 8, 8) if ($corrupt_header);
# Corrupt at least the line pointers. Exactly what this corrupts will
# depend on the page, as it may run past the line pointers into the user
# data. We stop short of writing 2048 bytes (2k), the smallest supported
# page size, as we don't want to corrupt the next page.
- seek($fh, 32, 0);- syswrite($fh, '\x77\x77\x77\x77', 500);- close($fh);+ seek($fh, 32, 0)+ or BAIL_OUT("seek failed: $!");+ syswrite_or_bail($fh, '\x77' x 2000, 2000);+ close($fh)+ or BAIL_OUT("close failed: $!");;
$node->start;
}
--
2.21.1 (Apple Git-122.3)
=
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: mark.dilger@enterprisedb.com, robertmhaas@gmail.com, tgl@sss.pgh.pa.us, pg@bowt.ie, x4mmm@yandex-team.ru, sfrost@snowman.net, michael@paquier.xyz, sulamul@gmail.com, dilipbalaut@gmail.com, andres@anarazel.de
Subject: Re: new heapcheck contrib module
In-Reply-To: <2A7DA1A8-C4AA-43DF-A985-3CA52F4DC775@enterprisedb.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox