From: Mark Dilger <mark.dilger@enterprisedb.com>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Noah Misch <noah@leadboat.com>
Cc: Peter Geoghegan <pg@bowt.ie>
Cc: Thomas Munro <thomas.munro@gmail.com>
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: pg_amcheck contrib application
Date: Thu, 8 Apr 2021 12:02:36 -0700
Message-ID: <E4F162F1-0DF4-4919-BD75-AF73F125D1E8@enterprisedb.com> (raw)
In-Reply-To: <CA+TgmoYw5YjGRt3hjg3XdZQ4rNnSB22JVdRO9SGE+MfjZayGZQ@mail.gmail.com>
References: <CAH2-WzmZJ3MWjaSEaysmkP8A7WL_TiCwOS8eT88yc-bwHgLOKg@mail.gmail.com>
<CAH2-WznpwULZ3uJ1_6WXvNMXYbOy8k8tYs3r=qSdGmZeRd6tDw@mail.gmail.com>
<CA+TgmoZUJ4j31CbnUz-DpBBq_DVoaYcuG5XB7hP71+uQ=js_KQ@mail.gmail.com>
<CA+TgmoZ1DP0pkLyuG9y7Jtn2v7N-+dnVxbvBAT__u4-d=UYmbw@mail.gmail.com>
<A80D68F6-E38F-482D-9522-E2FB6AAFE8A1@enterprisedb.com>
<CA+TgmoY3_U=QuQWo4DArBSp+tg2MiCHLwDN_9U2H_iTxj3UcjA@mail.gmail.com>
<9E76E46A-48B2-4869-BD0C-422204C1F767@enterprisedb.com>
<F0A1FD70-A2F4-4528-8A03-8650CAEC0554@enterprisedb.com>
<20210313184612.GA3195145@rfd.leadboat.com>
<930621E5-5D10-456D-8CE4-CF629A6C4669@enterprisedb.com>
<20210313191841.GB3195145@rfd.leadboat.com>
<3012106.1615827854@sss.pgh.pa.us>
<C2C142F3-F740-4197-B507-20B28B7395AC@enterprisedb.com>
<3053552.1615834640@sss.pgh.pa.us>
<AA5506CE-7D2A-42E4-A51D-358635E3722D@enterprisedb.com>
<CA+TgmoYAi_3_sCnsH2qwVFW1WUGqn3xOcGWjjvDtn4j4xfso+A@mail.gmail.com>
<4F10540A-9022-49EE-A6FC-7FE90797793E@enterprisedb.com>
<0D48534A-F511-405D-8645-84B0A1DA1E11@enterprisedb.com>
<CA+TgmoaA88XCjuFmzWnMh0fTcEWF_TWuXmzSAB0xKfXy84Fp3w@mail.gmail.com>
<CA+TgmobA_x0A1jh+V=vD1mHB+pzFaR2wTzng4yoKzgr_hvNOcg@mail.gmail.com>
<CA+Tgmoaq7PoO8XCRgaM3t43yer2bUc8Kt6ZsvE6wYtLPCTRsOA@mail.gmail.com>
<F42DEB1D-A3A9-4C6F-97D6-4B4C0ADAEF6C@enterprisedb.com>
<CA+TgmoZ3dkrFvtOraGgbH_uVQVjoOWQpmFDzq_EmcgmYwSfoPg@mail.gmail.com>
<6915B9B2-B551-4DBC-89DB-C12D9AF985B8@enterprisedb.com>
<CA+TgmoaHzOuFUzYKZ3rwJqJOOPH3P2c+M4AtLJeORpPdXBoTeA@mail.gmail.com>
<B88086A1-9A46-437E-9083-9077259E992E@enterprisedb.com>
<CA+Tgmob6sii0yTvULYJ0Vq4w6ZBmj7zUhddL3b+SKDi9z9NA7Q@mail.gmail.com>
<FCF74E43-318E-44C4-92E0-D03AB07588F3@enterprisedb.com>
<CA+TgmoZzy_Gn-3yh_iGiTz7dWWM0MmytKEFv5HoJWLko0cEVqA@mail.gmail.com>
<23192440-3B8A-4D82-8C24-9D8161BAAB01@enterprisedb.com>
<CA+TgmoYqfGfC-3=XajtV1BUZc3uKCgy0D5Mc0gz50V384unZqA@mail.gmail.com>
<1F1A6246-854F-47F0-A179-A3CD9F05356B@enterprisedb.com>
<CA+TgmoaUf5Yq+jN+rJdhSe+G4kzQywTk-LYmsPnCpTZZyZSVrg@mail.gmail.com>
<CA+TgmobpeiMh7Y+vve7eRv8Rru=7FJ_Z2nM8EMxew+sWBr58Og@mail.gmail.com>
<AC5479E4-6321-473D-AC92-5EC36299FBC2@enterprisedb.com>
<CA+TgmoYw5YjGRt3hjg3XdZQ4rNnSB22JVdRO9SGE+MfjZayGZQ@mail.gmail.com>
> On Apr 7, 2021, at 1:16 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Sun, Apr 4, 2021 at 8:02 PM Mark Dilger <mark.dilger@enterprisedb.com> wrote:
>> v18-0001 - Finishes work started in commit 3b6c1259f9 that was overlooked owing to how I had separated the changes in v17-0002 vs. v17-0003
>
> Committed.
Thank you.
>> v18-0002 - Postpones the toast checks for a page until after the main table page lock is released
>
> Committed, but I changed list_free() to list_free_deep() to avoid a
> memory leak, and I revised the commit message to mention the important
> point that we need to avoid following TOAST pointers from
> potentially-prunable tuples.
Thank you, and yes, I agree with that change.
>> v18-0003 - Improves the corruption messages in ways already discussed earlier in this thread. Changes the tests to expect the new messages, but adds no new checks
>
> Kibitizing your message wording:
>
> "toast value %u chunk data is null" -> "toast value %u chunk %d has
> null data". We can mention the chunk number this way.
Changed.
> "toast value %u corrupt extended chunk has invalid varlena header: %0x
> (sequence number %d)" -> "toast value %u chunk %d has invalid varlena
> header %0x". We can be more consistent about how we incorporate the
> chunk number into the text, and we don't really need to include the
> word corrupt, because all of these are corruption complaints, and I
> think it looks better without the colon.
Changed.
> "toast value %u chunk sequence number %u does not match the expected
> sequence number %u" -> "toast value %u contains chunk %d where chunk
> %d was expected". Shorter. Uses %d for a sequence number instead of
> %u, which I think is correct -- anyway we should have them all one way
> or all the other. I think I'd rather ditch the "sequence number"
> technology and just talk about "chunk %d" or whatever.
I don't agree with this one. I do agree with changing the message, but not to the message you suggest.
Imagine a toasted attribute with 18 chunks numbered [0..17]. Then we update the toast to have only 6 chunks numbered [0..5] except we corruptly keep chunks numbered [12..17] in the toast table. We'd rather see a report like this:
# heap table "postgres"."public"."test", block 0, offset 1, attribute 2:
# toast value 16444 chunk 6 has sequence number 12, but expected sequence number 6
# heap table "postgres"."public"."test", block 0, offset 1, attribute 2:
# toast value 16444 chunk 7 has sequence number 13, but expected sequence number 7
# heap table "postgres"."public"."test", block 0, offset 1, attribute 2:
# toast value 16444 chunk 8 has sequence number 14, but expected sequence number 8
# heap table "postgres"."public"."test", block 0, offset 1, attribute 2:
# toast value 16444 chunk 9 has sequence number 15, but expected sequence number 9
# heap table "postgres"."public"."test", block 0, offset 1, attribute 2:
# toast value 16444 chunk 10 has sequence number 16, but expected sequence number 10
# heap table "postgres"."public"."test", block 0, offset 1, attribute 2:
# toast value 16444 chunk 11 has sequence number 17, but expected sequence number 11
# heap table "postgres"."public"."test", block 0, offset 1, attribute 2:
# toast value 16444 was expected to end at chunk 6, but ended at chunk 12
than one like this:
# heap table "postgres"."public"."test", block 0, offset 1, attribute 2:
# toast value 16444 contains chunk 12 where chunk 6 was expected
# heap table "postgres"."public"."test", block 0, offset 1, attribute 2:
# toast value 16444 contains chunk 13 where chunk 7 was expected
# heap table "postgres"."public"."test", block 0, offset 1, attribute 2:
# toast value 16444 contains chunk 14 where chunk 8 was expected
# heap table "postgres"."public"."test", block 0, offset 1, attribute 2:
# toast value 16444 contains chunk 15 where chunk 9 was expected
# heap table "postgres"."public"."test", block 0, offset 1, attribute 2:
# toast value 16444 contains chunk 16 where chunk 10 was expected
# heap table "postgres"."public"."test", block 0, offset 1, attribute 2:
# toast value 16444 contains chunk 17 where chunk 11 was expected
# heap table "postgres"."public"."test", block 0, offset 1, attribute 2:
# toast value 16444 was expected to end at chunk 6, but ended at chunk 12
because saying the toast value ended at "chunk 12" after saying that it contains "chunk 17" is contradictory. You need the distinction between the chunk number and the chunk sequence number, since in corrupt circumstances they may not be the same.
> "toast value %u chunk sequence number %u exceeds the end chunk
> sequence number %u" -> "toast value %u chunk %d follows last expected
> chunk %d"
Changed.
> "toast value %u chunk size %u differs from the expected size %u" ->
> "toast value %u chunk %d has size %u, but expected size %u"
Changed.
> Other complaints:
>
> Your commit message fails to mention the addition of
> VARATT_EXTERNAL_GET_POINTER, which is a significant change/bug fix
> unrelated to message wording.
Right you are.
> It feels like we have a non-minimal number of checks/messages for the
> series of toast chunks. I think that if we find a chunk after the last
> chunk we were expecting to find (curchunk > endchunk) and you also get
> a message if we have the wrong number of chunks in total (chunkno !=
> (endchunk + 1)). Now maybe I'm wrong, but if the first message
> triggers, it seems like the second message must also trigger. Is that
> wrong? If not, maybe we can get rid of the first one entirely? That's
> such a small change I think we could include it in this same patch, if
> it's a correct idea.
Motivated by discussions we had off-list, I dug into this one.
Purely as manual testing, and not part of the patch, I hacked the backend a bit to allow direct modification of the toast table. After corrupting the toast with the following bit of SQL:
WITH chunk_limit AS (
SELECT chunk_id, MAX(chunk_seq) AS maxseq
FROM $toastname
GROUP BY chunk_id)
INSERT INTO $toastname (chunk_id, chunk_seq, chunk_data)
(SELECT t.chunk_id,
t.chunk_seq + cl.maxseq + CASE WHEN t.chunk_seq < 3 THEN 1 ELSE 7 END,
t.chunk_data
FROM $toastname t
INNER JOIN chunk_limit cl
ON t.chunk_id = cl.chunk_id)
pg_amcheck reports the following corruption messages:
# heap table "postgres"."public"."test", block 0, offset 1, attribute 2:
# toast value 16444 chunk 6 follows last expected chunk 5
# heap table "postgres"."public"."test", block 0, offset 1, attribute 2:
# toast value 16444 chunk 7 follows last expected chunk 5
# heap table "postgres"."public"."test", block 0, offset 1, attribute 2:
# toast value 16444 chunk 8 follows last expected chunk 5
# heap table "postgres"."public"."test", block 0, offset 1, attribute 2:
# toast value 16444 chunk 9 has sequence number 15, but expected sequence number 9
# heap table "postgres"."public"."test", block 0, offset 1, attribute 2:
# toast value 16444 chunk 10 has sequence number 16, but expected sequence number 10
# heap table "postgres"."public"."test", block 0, offset 1, attribute 2:
# toast value 16444 chunk 11 has sequence number 17, but expected sequence number 11
# heap table "postgres"."public"."test", block 0, offset 1, attribute 2:
# toast value 16444 was expected to end at chunk 6, but ended at chunk 12
I think if we'd left out the first three messages, it would read strangely. We would be complaining about three chunks with the wrong sequence number, then conclude that there were six extra chunks. A sufficiently savvy user might deduce the presence of chunks 6, 7, and 8, but the problem is more obvious (to my eyes, at least) if we keep the first three messages. This seems like a judgement call and not a clear argument either way, so if you still want me to change it, I guess I don't mind doing so.
> On a related note, as I think I said before, I still think we should
> be rejiggering this so that we're not testing both the size of each
> individual chunk and the total size, because that ought to be
> redundant. That might be better done as a separate patch but I think
> we should try to clean it up.
Can you point me to the exact check you are mentioning, and with which patch applied? I don't see any examples of this after applying the v18-0003.
>> v18-0004 - Adding corruption checks of toast pointers. Extends the regression tests to cover the new checks.
>
> I think we could check that the result of
> VARATT_EXTERNAL_GET_COMPRESS_METHOD is one of the values we expect to
> see.
Yes. I had that before, pulled it out along with other toast compression checks, but have put it back in for v19.
> Using AllocSizeIsValid() seems pretty vile. I know that MaxAllocSize
> is 0x3FFFFFFF in no small part because that's the maximum length that
> can be represented by a varlena, but I'm not sure it's a good idea to
> couple the concepts so closely like this. Maybe we can just #define
> VARLENA_SIZE_LIMIT in this file and use that, and a message that says
> size %u exceeds limit %u.
Changed.
> I'm a little worried about whether the additional test cases are
> Endian-dependent at all. I don't immediately know what might be wrong
> with them, but I'm going to think about that some more later. Any
> chance you have access to a Big-endian box where you can test this?
I don't have a Big-endian box, but I think one of them may be wrong now that you mention the issue:
# Corrupt column c's toast pointer va_extinfo field
The problem is that the 30-bit extsize and 2-bit cmid split is not being handled in the perl test, and I don't see an easy way to have perl's pack/unpack do that for us. There isn't any requirement that each possible corruption we check actually be manifested in the regression tests. The simplest solution is to remove this problematic test, so that's what I did. The other two new tests corrupt c_va_toastrelid and c_va_rawsize, both of which are read/written using unpack/pack, so perl should handle the endianness for us (I hope).
If you'd rather not commit these two extra tests, you don't have to, as I've split them out into v19-0003. But if you do commit them, it makes more sense to me to be one commit with 0002+0003 together, rather than separately. Not committing the new tests just means that verify_heapam() is able to detect additional forms of corruption that we're not covering in the regression tests. But that's already true for some other corruption types, such as detecting toast chunks with null sequence numbers.
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachments:download all (3 files, .tar.gz)v19-0001-amcheck-rewording-messages-and-fixing-alignment.patch (application/octet-stream, 8.1K) download
From b6ea48417cc99258bf2f9106feed4bfebfcf8212 Mon Sep 17 00:00:00 2001
From: Mark Dilger <mark.dilger@enterprisedb.com>
Date: Thu, 8 Apr 2021 09:48:37 -0700
Subject: [PATCH v19 1/3] amcheck: rewording messages and fixing alignment
Removing redundant mention of attnum in the corruption message text,
as the attnum is already its own separate column.
When reporting toast corruption, mentioning the toast value in the
message since that information is not otherwise reported.
Being more careful about alignment when accessing a toast pointer.
---
contrib/amcheck/verify_heapam.c | 63 +++++++++++++----------
src/bin/pg_amcheck/t/004_verify_heapam.pl | 4 +-
2 files changed, 38 insertions(+), 29 deletions(-)
diff --git a/contrib/amcheck/verify_heapam.c b/contrib/amcheck/verify_heapam.cindex e8aa0d68d4..13f420d9ad 100644--- a/contrib/amcheck/verify_heapam.c+++ b/contrib/amcheck/verify_heapam.c@@ -1179,7 +1179,8 @@ check_toast_tuple(HeapTuple toasttup, HeapCheckContext *ctx,
if (isnull)
{
report_toast_corruption(ctx, ta,
- pstrdup("toast chunk sequence number is null"));+ psprintf("toast value %u has toast chunk with null sequence number",+ ta->toast_pointer.va_valueid));
return;
}
chunk = DatumGetPointer(fastgetattr(toasttup, 3,
@@ -1187,7 +1188,8 @@ check_toast_tuple(HeapTuple toasttup, HeapCheckContext *ctx,
if (isnull)
{
report_toast_corruption(ctx, ta,
- pstrdup("toast chunk data is null"));+ psprintf("toast value %u chunk %d has null data",+ ta->toast_pointer.va_valueid, chunkno));
return;
}
if (!VARATT_IS_EXTENDED(chunk))
@@ -1205,8 +1207,9 @@ check_toast_tuple(HeapTuple toasttup, HeapCheckContext *ctx,
uint32 header = ((varattrib_4b *) chunk)->va_4byte.va_header;
report_toast_corruption(ctx, ta,
- psprintf("corrupt extended toast chunk has invalid varlena header: %0x (sequence number %d)",- header, curchunk));+ psprintf("toast value %u chunk %d has invalid varlena header %0x",+ ta->toast_pointer.va_valueid,+ chunkno, header));
return;
}
@@ -1216,15 +1219,17 @@ check_toast_tuple(HeapTuple toasttup, HeapCheckContext *ctx,
if (curchunk != chunkno)
{
report_toast_corruption(ctx, ta,
- psprintf("toast chunk sequence number %u does not match the expected sequence number %u",- curchunk, chunkno));+ psprintf("toast value %u chunk %d has sequence number %d, but expected sequence number %d",+ ta->toast_pointer.va_valueid,+ chunkno, curchunk, chunkno));
return;
}
- if (curchunk > endchunk)+ if (chunkno > endchunk)
{
report_toast_corruption(ctx, ta,
- psprintf("toast chunk sequence number %u exceeds the end chunk sequence number %u",- curchunk, endchunk));+ psprintf("toast value %u chunk %d follows last expected chunk %d",+ ta->toast_pointer.va_valueid,+ chunkno, endchunk));
return;
}
@@ -1233,8 +1238,9 @@ check_toast_tuple(HeapTuple toasttup, HeapCheckContext *ctx,
if (chunksize != expected_size)
report_toast_corruption(ctx, ta,
- psprintf("toast chunk size %u differs from the expected size %u",- chunksize, expected_size));+ psprintf("toast value %u chunk %d has size %u, but expected size %u",+ ta->toast_pointer.va_valueid,+ chunkno, chunksize, expected_size));
}
/*
@@ -1265,6 +1271,7 @@ check_tuple_attribute(HeapCheckContext *ctx)
char *tp; /* pointer to the tuple data */
uint16 infomask;
Form_pg_attribute thisatt;
+ struct varatt_external toast_pointer;
infomask = ctx->tuphdr->t_infomask;
thisatt = TupleDescAttr(RelationGetDescr(ctx->rel), ctx->attnum);
@@ -1274,8 +1281,7 @@ check_tuple_attribute(HeapCheckContext *ctx)
if (ctx->tuphdr->t_hoff + ctx->offset > ctx->lp_len)
{
report_corruption(ctx,
- psprintf("attribute %u with length %u starts at offset %u beyond total tuple length %u",- ctx->attnum,+ psprintf("attribute with length %u starts at offset %u beyond total tuple length %u",
thisatt->attlen,
ctx->tuphdr->t_hoff + ctx->offset,
ctx->lp_len));
@@ -1295,8 +1301,7 @@ check_tuple_attribute(HeapCheckContext *ctx)
if (ctx->tuphdr->t_hoff + ctx->offset > ctx->lp_len)
{
report_corruption(ctx,
- psprintf("attribute %u with length %u ends at offset %u beyond total tuple length %u",- ctx->attnum,+ psprintf("attribute with length %u ends at offset %u beyond total tuple length %u",
thisatt->attlen,
ctx->tuphdr->t_hoff + ctx->offset,
ctx->lp_len));
@@ -1328,8 +1333,7 @@ check_tuple_attribute(HeapCheckContext *ctx)
if (va_tag != VARTAG_ONDISK)
{
report_corruption(ctx,
- psprintf("toasted attribute %u has unexpected TOAST tag %u",- ctx->attnum,+ psprintf("toasted attribute has unexpected TOAST tag %u",
va_tag));
/* We can't know where the next attribute begins */
return false;
@@ -1343,8 +1347,7 @@ check_tuple_attribute(HeapCheckContext *ctx)
if (ctx->tuphdr->t_hoff + ctx->offset > ctx->lp_len)
{
report_corruption(ctx,
- psprintf("attribute %u with length %u ends at offset %u beyond total tuple length %u",- ctx->attnum,+ psprintf("attribute with length %u ends at offset %u beyond total tuple length %u",
thisatt->attlen,
ctx->tuphdr->t_hoff + ctx->offset,
ctx->lp_len));
@@ -1371,12 +1374,17 @@ check_tuple_attribute(HeapCheckContext *ctx)
/* It is external, and we're looking at a page on disk */
+ /*+ * Must copy attr into toast_pointer for alignment considerations+ */+ VARATT_EXTERNAL_GET_POINTER(toast_pointer, attr);+
/* The tuple header better claim to contain toasted values */
if (!(infomask & HEAP_HASEXTERNAL))
{
report_corruption(ctx,
- psprintf("attribute %u is external but tuple header flag HEAP_HASEXTERNAL not set",- ctx->attnum));+ psprintf("toast value %u is external but tuple header flag HEAP_HASEXTERNAL not set",+ toast_pointer.va_valueid));
return true;
}
@@ -1384,8 +1392,8 @@ check_tuple_attribute(HeapCheckContext *ctx)
if (!ctx->rel->rd_rel->reltoastrelid)
{
report_corruption(ctx,
- psprintf("attribute %u is external but relation has no toast relation",- ctx->attnum));+ psprintf("toast value %u is external but relation has no toast relation",+ toast_pointer.va_valueid));
return true;
}
@@ -1464,12 +1472,13 @@ check_toasted_attribute(HeapCheckContext *ctx, ToastedAttribute *ta)
if (!found_toasttup)
report_toast_corruption(ctx, ta,
- psprintf("toasted value for attribute %u missing from toast table",- ta->attnum));+ psprintf("toast value %u not found in toast table",+ ta->toast_pointer.va_valueid));
else if (chunkno != (endchunk + 1))
report_toast_corruption(ctx, ta,
- psprintf("final toast chunk number %u differs from expected value %u",- chunkno, (endchunk + 1)));+ psprintf("toast value %u was expected to end at chunk %u, but ended at chunk %u",+ ta->toast_pointer.va_valueid,+ (endchunk + 1), chunkno));
}
/*
diff --git a/src/bin/pg_amcheck/t/004_verify_heapam.pl b/src/bin/pg_amcheck/t/004_verify_heapam.plindex 36607596b1..307f14611c 100644--- a/src/bin/pg_amcheck/t/004_verify_heapam.pl+++ b/src/bin/pg_amcheck/t/004_verify_heapam.pl@@ -480,7 +480,7 @@ for (my $tupidx = 0; $tupidx < ROWCOUNT; $tupidx++)
$header = header(0, $offnum, 1);
push @expected,
- qr/${header}attribute \d+ with length \d+ ends at offset \d+ beyond total tuple length \d+/;+ qr/${header}attribute with length \d+ ends at offset \d+ beyond total tuple length \d+/;
}
elsif ($offnum == 13)
{
@@ -489,7 +489,7 @@ for (my $tupidx = 0; $tupidx < ROWCOUNT; $tupidx++)
$header = header(0, $offnum, 2);
push @expected,
- qr/${header}toasted value for attribute 2 missing from toast table/;+ qr/${header}toast value \d+ not found in toast table/;
}
elsif ($offnum == 14)
{
--
2.21.1 (Apple Git-122.3)
=
v19-0002-amcheck-adding-toast-pointer-corruption-checks.patch (application/octet-stream, 4.2K) download
From 86addc39596cf980a5850d9b1dba23fdaa927485 Mon Sep 17 00:00:00 2001
From: Mark Dilger <mark.dilger@enterprisedb.com>
Date: Thu, 8 Apr 2021 10:09:59 -0700
Subject: [PATCH v19 2/3] amcheck: adding toast pointer corruption checks
Verifying that toast pointer va_toastrelid fields match their heap
table's reltoastrelid.
Checking the extsize for a toast pointer against the raw size. This
check could fail if buggy compression logic fails to notice that
compressing the attribute makes it bigger. But assuming the logic
for that is correct, overlarge extsize indicates a corrupted toast
pointer.
Checking if a toast pointer indicates the data is compressed, that
the toast pointer records a valid compression method.
Checking the toast is not too large to be allocated. No such
toasted value should ever be stored, but a corrupted toast pointer
could record an unreasonbly large size, so check that.
Changing the logic to continue checking toast even after reporting
that HEAP_HASEXTERNAL is false. Previously, the toast checking
stopped here, but that wasn't necessary, and subsequent checks may
provide additional useful diagnostic information.
---
contrib/amcheck/verify_heapam.c | 56 +++++++++++++++++++++++++++++++--
1 file changed, 53 insertions(+), 3 deletions(-)
diff --git a/contrib/amcheck/verify_heapam.c b/contrib/amcheck/verify_heapam.cindex 13f420d9ad..7b7b40f415 100644--- a/contrib/amcheck/verify_heapam.c+++ b/contrib/amcheck/verify_heapam.c@@ -30,6 +30,8 @@ PG_FUNCTION_INFO_V1(verify_heapam);
/* The number of columns in tuples returned by verify_heapam */
#define HEAPCHECK_RELATION_COLS 4
+#define VARLENA_SIZE_LIMIT 0x3FFFFFFF+
/*
* Despite the name, we use this for reporting problems with both XIDs and
* MXIDs.
@@ -1379,14 +1381,54 @@ check_tuple_attribute(HeapCheckContext *ctx)
*/
VARATT_EXTERNAL_GET_POINTER(toast_pointer, attr);
+ if (toast_pointer.va_rawsize > VARLENA_SIZE_LIMIT)+ report_corruption(ctx,+ psprintf("toast value %u rawsize %u exceeds limit %u",+ toast_pointer.va_valueid,+ toast_pointer.va_rawsize,+ VARLENA_SIZE_LIMIT));++ /* Compression should never expand the attribute */+ if (VARATT_EXTERNAL_GET_EXTSIZE(toast_pointer) > toast_pointer.va_rawsize - VARHDRSZ)+ report_corruption(ctx,+ psprintf("toast value %u external size %u exceeds maximum expected for rawsize %u",+ toast_pointer.va_valueid,+ VARATT_EXTERNAL_GET_EXTSIZE(toast_pointer),+ toast_pointer.va_rawsize));++ /* Compressed attributes should have a valid compression method */+ if (VARATT_IS_COMPRESSED(&toast_pointer))+ {+ ToastCompressionId cmid;+ bool invalid = true;++ cmid = TOAST_COMPRESS_METHOD(&toast_pointer);+ switch (cmid)+ {+ /* List of all valid compression method IDs */+ case TOAST_PGLZ_COMPRESSION_ID:+ case TOAST_LZ4_COMPRESSION_ID:+ invalid = false;+ break;++ /* Recognized but invalid compression method ID */+ case TOAST_INVALID_COMPRESSION_ID:+ break;++ /* Intentionally no default here */+ }++ if (invalid)+ report_corruption(ctx,+ psprintf("toast value %u has invalid compression method id %d",+ toast_pointer.va_valueid, cmid));+ }+
/* The tuple header better claim to contain toasted values */
if (!(infomask & HEAP_HASEXTERNAL))
- {
report_corruption(ctx,
psprintf("toast value %u is external but tuple header flag HEAP_HASEXTERNAL not set",
toast_pointer.va_valueid));
- return true;- }
/* The relation better have a toast table */
if (!ctx->rel->rd_rel->reltoastrelid)
@@ -1397,6 +1439,14 @@ check_tuple_attribute(HeapCheckContext *ctx)
return true;
}
+ /* The toast pointer had better point at the relation's toast table */+ if (toast_pointer.va_toastrelid != ctx->rel->rd_rel->reltoastrelid)+ report_corruption(ctx,+ psprintf("toast value %u toast relation oid %u differs from expected oid %u",+ toast_pointer.va_valueid,+ toast_pointer.va_toastrelid,+ ctx->rel->rd_rel->reltoastrelid));+
/* If we were told to skip toast checking, then we're done. */
if (ctx->toast_rel == NULL)
return true;
--
2.21.1 (Apple Git-122.3)
=
v19-0003-amcheck-additional-regression-test-coverage.patch (application/octet-stream, 3.3K) download
From cd3534caf75aeb8397c7754a26a8e3722508bbd2 Mon Sep 17 00:00:00 2001
From: Mark Dilger <mark.dilger@enterprisedb.com>
Date: Thu, 8 Apr 2021 10:24:40 -0700
Subject: [PATCH v19 3/3] amcheck: additional regression test coverage
Adding regression checks for two corruption checks added in prior
commit. We now verify that corruption reports are generated when we
set a toast pointer's va_toastrelid to the wrong Oid or set a toast
pointer's va_rawsize to an overlarge value.
---
src/bin/pg_amcheck/t/004_verify_heapam.pl | 31 ++++++++++++++++++++---
1 file changed, 28 insertions(+), 3 deletions(-)
diff --git a/src/bin/pg_amcheck/t/004_verify_heapam.pl b/src/bin/pg_amcheck/t/004_verify_heapam.plindex 307f14611c..e64801c7e1 100644--- a/src/bin/pg_amcheck/t/004_verify_heapam.pl+++ b/src/bin/pg_amcheck/t/004_verify_heapam.pl@@ -224,7 +224,7 @@ my $rel = $node->safe_psql('postgres', qq(SELECT pg_relation_filepath('public.te
my $relpath = "$pgdata/$rel";
# Insert data and freeze public.test
-use constant ROWCOUNT => 16;+use constant ROWCOUNT => 18;
$node->safe_psql('postgres', qq(
INSERT INTO public.test (a, b, c)
VALUES (
@@ -240,6 +240,13 @@ my $relfrozenxid = $node->safe_psql('postgres',
my $datfrozenxid = $node->safe_psql('postgres',
q(select datfrozenxid from pg_database where datname = 'postgres'));
+# Find our toast relation id+my $toastrelid = $node->safe_psql('postgres', qq(+ SELECT c.reltoastrelid+ FROM pg_catalog.pg_class c+ WHERE c.oid = 'public.test'::regclass+ ));+
# Sanity check that our 'test' table has a relfrozenxid newer than the
# datfrozenxid for the database, and that the datfrozenxid is greater than the
# first normal xid. We rely on these invariants in some of our tests.
@@ -296,7 +303,7 @@ close($file)
$node->start;
# Ok, Xids and page layout look ok. We can run corruption tests.
-plan tests => 19;+plan tests => 22;
# Check that pg_amcheck runs against the uncorrupted table without error.
$node->command_ok(['pg_amcheck', '-p', $port, 'postgres'],
@@ -501,7 +508,7 @@ for (my $tupidx = 0; $tupidx < ROWCOUNT; $tupidx++)
push @expected,
qr/${header}multitransaction ID 4 equals or exceeds next valid multitransaction ID 1/;
}
- elsif ($offnum == 15) # Last offnum must equal ROWCOUNT+ elsif ($offnum == 15)
{
# Set both HEAP_XMAX_COMMITTED and HEAP_XMAX_IS_MULTI
$tup->{t_infomask} |= HEAP_XMAX_COMMITTED;
@@ -511,6 +518,24 @@ for (my $tupidx = 0; $tupidx < ROWCOUNT; $tupidx++)
push @expected,
qr/${header}multitransaction ID 4000000000 precedes relation minimum multitransaction ID threshold 1/;
}
+ elsif ($offnum == 16)+ {+ # Corrupt column c's toast pointer va_toastrelid field+ my $otherid = $toastrelid + 1;+ $tup->{c_va_toastrelid} = $otherid;+ $header = header(0, $offnum, 2);+ push @expected,+ qr/${header}toast value \d+ toast relation oid $otherid differs from expected oid $toastrelid/;+ }+ elsif ($offnum == 17) # Last offnum should equal ROWCOUNT-1+ {+ # Corrupt column c's toast pointer va_rawsize field with a value+ # exceeding maximum allowable allocation size+ $tup->{c_va_rawsize} = 0x40000000;+ $header = header(0, $offnum, 2);+ push @expected,+ qr/${header}toast value \d+ rawsize 1073741824 exceeds limit 1073741823/;+ }
write_tuple($file, $offset, $tup);
}
close($file)
--
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, noah@leadboat.com, pg@bowt.ie, thomas.munro@gmail.com, x4mmm@yandex-team.ru, sfrost@snowman.net, michael@paquier.xyz, sulamul@gmail.com, dilipbalaut@gmail.com, andres@anarazel.de
Subject: Re: pg_amcheck contrib application
In-Reply-To: <E4F162F1-0DF4-4919-BD75-AF73F125D1E8@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