Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1lYe50-0004Op-5i for pgsql-hackers@arkaria.postgresql.org; Tue, 20 Apr 2021 00:17:26 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1lYdw1-00047p-3i for pgsql-hackers@arkaria.postgresql.org; Tue, 20 Apr 2021 00:08:09 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1lYdw0-00047i-MU for pgsql-hackers@lists.postgresql.org; Tue, 20 Apr 2021 00:08:08 +0000 Received: from mail-pf1-x42c.google.com ([2607:f8b0:4864:20::42c]) by magus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.92) (envelope-from ) id 1lYdvw-00028W-C0 for pgsql-hackers@postgresql.org; Tue, 20 Apr 2021 00:08:07 +0000 Received: by mail-pf1-x42c.google.com with SMTP id p67so19363538pfp.10 for ; Mon, 19 Apr 2021 17:08:03 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=enterprisedb-com.20150623.gappssmtp.com; s=20150623; h=mime-version:subject:from:in-reply-to:date:cc :content-transfer-encoding:message-id:references:to; bh=vHM3kkZ+AgSFmOEtebSR9uPTRJLH1kEwsvIcmpReyck=; b=oI2Ypcf0CsHpKKAIKO2NH1HVoTGNy2c1hwhS9oM1LDrYycL0O10vqeOd7huw3uMYGr NJDkDmcye2MdLFQWvfoWGgK9bC921eDnHCHJVEhhnUdeyTzPUQz/uLwhWf17RTJo4JWl DfHkjzyS0jje2klPw9wL5AjWJ+4+bkHNSYTVZGd7kLmsL+tw2uMmo9JwYx2vbC4t9dIJ yH5600KF8XyW+xTCBkPmB2FneaXQpVDC6Sg8zFh5Kx73rR0gyesEHB/ryGjdJWc1RPMQ jfuA6YRbjU+1MsuGNw9Q2AiaDzzonAUC9pAClPtpqi6buOFIqZ/1Vy0xicOyim88otRf Q4uA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:subject:from:in-reply-to:date:cc :content-transfer-encoding:message-id:references:to; bh=vHM3kkZ+AgSFmOEtebSR9uPTRJLH1kEwsvIcmpReyck=; b=gqhSXt3SQEgUrngtPKHUdWOzj8/ByPlx5tiiyoE6E1koodbHc6BQQOTefcGUAkF0rT kFP6yTWrDizJJSOnGdrxbX7aGKCv/+mI4qFhMulYWdUh+XlxqotgyRc5wWHhYw79VS5g bs7fRBFKPSn2GAFHCghRY1t/wXj5wdqo/1VrU1peKuP+1aNCn2SH25w01aFl+gYOKCEz ukfoqRLb+eHx5ZGw9fKtnCzBmi1qtLS4m+5b9BvhbDP1+1udpKSAY6QiRtA2J5NxFxpi NtkrHuhTIFXm9dgbypn3Oc66AAbRc2eZjdzNo7FU3gW3nLlvRFSpIbxENOcVztbfhbbg GP4Q== X-Gm-Message-State: AOAM533dyM2EuyA3Wu8uGVkbQTodHZ4WuvWbbcvN57VYgcNi8URClQ6b 54qG9Djnb7iI4LqeSsoVYDIOqVxdZ0anXeGSiBfpCsiEHktA3cSZPW+ZLeRMtNlK44LbdtUwLV1 IDbRyyihACEK+m2GpZ4TmqVI07Lhgdaj1eXExXJgeUvrAD0muerbZ5I7yoqjLWxM9E+0YnIOXhS 5He/NemLvvuBlMRC72u1NFTCmm8szfgAHLSdZ5 X-Google-Smtp-Source: ABdhPJxXYEU+2PPIFP3uNdKh/KW2HDRo6750bEfmG4XpiPB0AiyxlaeNUQdMbv5cq4dWSl9kDgi9xA== X-Received: by 2002:a63:af5d:: with SMTP id s29mr14156570pgo.220.1618877280448; Mon, 19 Apr 2021 17:08:00 -0700 (PDT) Received: from laptop280-ma-us.hitronhub.home (24-113-193-150.wavecable.com. [24.113.193.150]) by smtp.gmail.com with ESMTPSA id y18sm4756469pfp.121.2021.04.19.17.07.58 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Mon, 19 Apr 2021 17:07:59 -0700 (PDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.4\)) Subject: Re: pg_amcheck contrib application From: Mark Dilger In-Reply-To: Date: Mon, 19 Apr 2021 17:07:58 -0700 Cc: Tom Lane , Noah Misch , Peter Geoghegan , Thomas Munro , "Andrey M. Borodin" , Stephen Frost , Michael Paquier , Amul Sul , Dilip Kumar , Andres Freund , PostgreSQL-development Content-Transfer-Encoding: quoted-printable Message-Id: References: <9E76E46A-48B2-4869-BD0C-422204C1F767@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> <3053552.1615834640@sss.pgh.pa.us> <4F10540A-9022-49EE-A6FC-7FE90797793E@enterprisedb.com> <0D48534A-F511-405D-8645-84B0A1DA1E11@enterprisedb.com> <6915B9B2-B551-4DBC-89DB-C12D9AF985B8@enterprisedb.com> <23192440-3B8A-4D82-8C24-9D8161BAAB01@enterprisedb.com> <1F1A6246-854F-47F0-A179-A3CD9F05356B@enterprisedb.com> <50DA1F9A-D6AB-40DE-AB24-E8BDC13F4DA1@enterprisedb.com> <7D3B9BF6-50D0-4C30-8506-1C1851C7F96F@enterprisedb.com> To: Robert Haas X-Mailer: Apple Mail (2.3608.120.23.2.4) X-CLOUD-SEC-AV-Info: enterprisedb,google_mail,monitor X-CLOUD-SEC-AV-Sent: true X-Gm-Spam: 0 X-Gm-Phishy: 0 List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk > On Apr 19, 2021, at 12:50 PM, Robert Haas = wrote: >=20 > On Thu, Apr 15, 2021 at 1:07 PM Mark Dilger > wrote: >> I have added the verb "has" rather than "contains" because "has" is = more consistent with the phrasing of other similar corruption reports. >=20 > That makes sense. >=20 > I think it's odd that a range of extraneous chunks is collapsed into a > single report if the size of each chunk happens to be > TOAST_MAX_CHUNK_SIZE and not otherwise. Why not just remember the > first and last extraneous chunk and the size of each? If the next > chunk you see is the next one in sequence and the same size as all the > others, extend your notion of the sequence end by 1. Otherwise, report > the range accumulated so far. It seems to me that this wouldn't be any > more code than you have now, and might actually be less. In all cases of uncorrupted toasted attributes, the sequence of N chunks = that make up the attribute should be N-1 chunks of TOAST_MAX_CHUNK_SIZE = ending with a single chunk of up to TOAST_MAX_CHUNK_SIZE. I'd like to = refer to such sequences as "reasonably sized" sequences to make = conversation easier. If the toast pointer's va_extsize field leads us to believe that we = should find 10 reasonably sized chunks, but instead we find 30 = reasonably sized chunks, we know something is corrupt. We shouldn't = automatically prejudice the user against the additional 20 chunks. We = didn't expect them, but maybe that's because va_extsize was corrupt and = gave us a false expectation. We're not pointing fingers one way or the = other. On the other hand, if we expect 10 chunks and find an additional 20 = unreasonably sized chunks, we can and should point fingers at the extra = 20 chunks. Even if we somehow knew that va_extsize was also corrupt, = we'd still be justified in saying the 20 unreasonably sized chunks are = each, individually corrupt. I tried to write the code to report one corruption message per = corruption found. There are some edge cases where this is a = definitional challenge, so it's not easy to say that I've always = achieved this goal, but I think i've done so where the definitions are = clear. As such, the only time I'd want to combine toast chunks into a = single corruption message is when they are not in themselves necessarily = *individually* corrupt. That is why I wrote the code to use = TOAST_MAX_CHUNK_SIZE rather than just storing up any series of equally = sized chunks. On a related note, when complaining about a sequence of toast chunks, = often the sequence is something like [maximal, maximal, ..., maximal, = partial], but sometimes it's just [maximal...maximal], sometimes just = [maximal], and sometimes just [partial]. If I'm complaining about that = entire sequence, I'd really like to do so in just one message, otherwise = it looks like separate complaints. I can certainly change the code to be how you are asking, but I'd first = like to know that you really understood what I was doing here and why = the reports read the way they do. > I think that report_missing_chunks() could probably just report the > range of missing chunks and not bother reporting how big they were > expected to be. But, if it is going to report how big they were > expected to be, I think it should have only 2 cases rather than 4: > either a range of missing chunks of equal size, or a single missing > chunk of some size. If, as I propose, it doesn't report the expected > size, then you still have just 2 cases: a range of missing chunks, or > a single missing chunk. Right, this is the same as above. I'm trying not to split a single = corruption complaint into separate reports. > Somehow I have a hard time feeling confident that check_toast_tuple() > is going to do the right thing. The logic is really complex and hard > to understand. 'chunkno' is a counter that advances every time we move > to the next chunk, and 'curchunk' is the value we actually find in the > TOAST tuple. This terminology is not easy to understand. Most messages > now report 'curchunk', but some still report 'chunkno'. Why does > 'chunkno' need to exist at all? AFAICS the combination of 'curchunk' > and 'tctx->last_chunk_seen' ought to be sufficient. I can see no > particular reason why what you're calling 'chunkno' needs to exist > even as a local variable, let alone be printed out. Either we haven't > yet validated that the chunk_id extracted from the tuple is non-null > and greater than the last chunk number we saw, in which case we can > just complain about it if we find it to be otherwise, or we have > already done that validation, in which case we should complain about > that value and not 'chunkno' in any subsequent messages. If we use tctx->last_chunk_seen as you propose, I imagine we'd set that = to -1 prior to the first call to check_toast_tuple(). In the first = call, we'd extract the toast chunk_seq and store it in curchunk and = verify that it's one greater than tctx->last_chunk_seen. That all seems = fine. But under corrupt conditions, curchunk =3D = DatumGetInt32(fastgetattr(toasttup, 2, hctx->toast_rel->rd_att, = &isnull)) could return -1. That's invalid, of course, but now we don't = know what to do. We're supposed to complain when we get the same = chunk_seq from the index scan more than once in a row, but we don't know = if the value in last_chunk_seen is a real value or just the dummy = initial value. Worse still, when we get the next toast tuple back and = it has a chunk_seq of -2, we want to complain that the index is = returning tuples in reverse order, but we can't, because we still don't = know if the -1 in last_chunk_seen is legitimate or a dummy value because = that state information isn't carried over from the previous call. Using chunkno solves this problem. If chunkno =3D=3D 0, it means this = is our first call, and tctx->last_chunk_seen is uninitialized. = Otherwise, this is not the first call, and tctx->last_chunk_seen really = is the chunk_seq seen in the prior call. There is no ambiguity. I could probably change "int chunkno" to "bool is_first_call" or = similar. I had previously used chunkno in the corruption report about = chunks whose chunk_seq is null. The idea was that if you have 100 = chunks and the 30th chunk is corruptly nulled out, you could say = something like "toast value 178337 has toast chunk 30 with null sequence = number", but you had me change that to "toast value 178337 has toast = chunk with null sequence number", so generation of that message no = longer needs the chunkno. I had kept chunkno around for the other = purpose of knowing whether tctx->last_chunk_seen has been initialized = yet, but a bool for that would now be sufficient. In any event, though = you disagree with me about this below, I think the caller of this code = still needs to track chunkno. > The conditionals between where you set max_valid_prior_chunk and where > you set last_chunk_seen seem hard to understand, particularly the > bifurcated way that missing chunks are reported. Initial missing > chunks are detected by (chunkno =3D=3D 0 && max_valid_prior_chunk >=3D = 0) > and later missing chunks are detected by (chunkno > 0 && > max_valid_prior_chunk > tctx->last_chunk_seen). I'm not sure if this > is correct; When we read a chunk_seq from a toast tuple, we need to determine if it = indicates a gap in the chunk sequence, but we need to be careful.=20 The (chunkno =3D=3D 0) and (chunkno > 0) stuff is just distinguishing = between the first call and all subsequent calls. For illustrative purposes, imagine that we expect chunks [0..4]. On the first call, we expect chunk_seq =3D 0, but that's not what we = actually complain about if we get chunk_seq =3D 15. We complain about = all missing expected chunks, namely [0..4], not [0..14]. We also don't = complain yet about seeing extraneous chunk 15, because it might be the = first in a series of contiguous extraneous chunks, and we want to wait = and report those all at once when the sequence finishes. Simply = complaining at this point that we didn't expect to see chunk_seq 15 is = the kind of behavior that we already have committed and are trying to = fix because the corruption reports are not on point. On subsequent calls, we expect chunk_seq =3D last_chunk_seen+1, but = that's also not what we actually complain about if we get some other = value for chunk_seq. What we complain about are the missing and = extraneous sequences, not the individual chunk that had an unexpected = value. > I find it hard to get my head around what > max_valid_prior_chunk is supposed to represent. But in any case I > think it can be written more simply. Just keep track of what chunk_id > we expect to extract from the next TOAST tuple. Initially it's 0. > Then: >=20 > if (chunkno < tctx->expected_chunkno) > { > // toast value %u index scan returned chunk number %d when chunk %d > was expected > // don't modify tctx->expected_chunkno here, just hope the next > thing matches our previous expectation > } > else > { > if (chunkno > tctx->expected_chunkno) > // chunks are missing from tctx->expected_chunkno through > Min(chunkno - 1, tctx->final_expected_chunk), provided that the latter > value is greater than or equal to the former > tctx->expected_chunkno =3D chunkno + 1; > } >=20 > If you do this, you only need to report extraneous chunks when chunkno >> tctx->final_expected_chunk, since chunkno < 0 is guaranteed to > trigger the first of the two complaints shown above. In the example above, if we're expecting chunks [0..4] and get chunk_seq = =3D 5, the max_valid_prior_chunk is 4. If we instead get chunk_seq =3D = 6, the max_valid_prior_chunk is still 4, because chunk 5 is out of = bounds. > In check_tuple_attribute I suggest "bool valid =3D false" rather than > "bool invalid =3D true". I think it's easier to understand. Yeah, I had it that way and changed it, because I don't much like having = the only use of a boolean be a negation. bool foo =3D false; ... if (!foo) { ... } seems worse to me than bool foo =3D true; ... if (foo) { ... } But you're looking at it more from the perspective of english grammar, = where "invalid =3D false" reads as a double-negative. That's fine. I = can change it back. > I object to check_toasted_attribute() using 'chunkno' in a message for > the same reasons as above in regards to check_toast_tuple() i.e. I > think it's a concept which should not exist. So if we expect 100 chunks, get chunks [0..19, 80..99], you'd have me = write the message as "expected 100 chunks but sequence ended at chunk = 99"? I think that's odd. It makes infinitely more sense to me to say = "expected 100 chunks but sequence ended at chunk 40". Actually, this is = an argument against changing "int chunkno" to "bool is_first_call", as I = alluded to above, because we have to keep the chunkno around anyway. > I think this patch could possibly be split up into multiple patches. > There's some question in my mind whether it's getting too late to > commit any of this, since some of it looks suspiciously like new > features after feature freeze. However, I kind of hate to ship this > release without at least doing something about the chunkno vs. > curchunk stuff, which is even worse in the committed code than in your > patch, and which I think will confuse the heck out of users if those > messages actually fire for anyone. I'm in favor of cleaning up the committed code to have easier to = understand output. I don't really agree with any of your proposed = changes to my patch, though, which is I think a first. =E2=80=94 Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company