Re: pg_amcheck contrib application

> On Apr 1, 2021, at 1:08 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
>
>
> - There are a total of two (2) calls in the current source code to
> palloc0fast, and hundreds of calls to palloc0. So I think you should
> forget about using the fast variant and just do what almost every
> other caller does.

Done.

> - If you want to make this code faster, a better idea would be to
> avoid doing all of this allocate and free work and just allocate an
> array that's guaranteed to be big enough, and then keep track of how
> many elements of that array are actually in use.

Sounds like premature optimization to me. I only used palloc0fast because the argument is compile-time known. I wasn't specifically attempting to speed anything up.

> - #ifdef DECOMPRESSION_CORRUPTION_CHECKING is not a useful way of
> introducing such a feature. Either we do it for real and expose it via
> SQL and pg_amcheck as an optional behavior, or we rip it out and
> revisit it later. Given the nearness of feature freeze, my vote is for
> the latter.
>
> - I'd remove the USE_LZ4 bit, too. Let's not define the presence of
> LZ4 data in a non-LZ4-enabled cluster as corruption. If we do, then
> people will expect to be able to use this to find places where they
> are dependent on LZ4 if they want to move away from it -- and if we
> don't recurse into composite datums, that will not actually work.

Ok, I have removed this bit. I also removed the part of the patch that introduced a new corruption check, decompressing the data to see if it decompresses without error.

> - check_toast_tuple() has an odd and slightly unclear calling
> convention for which there are no comments. I wonder if it would be
> better to reverse things and make bool *error the return value and
> what is now the return value into a pointer argument, but whatever we
> do I think it needs a few words in the comments. We don't need to
> slavishly explain every argument -- I think toasttup and ctx and tctx
> are reasonably clear -- but this is not.
...
> - Is there a reason we need a cross-check on both the number of chunks
> and on the total size? It seems to me that we should check that each
> individual chunk has the size we expect, and that the total number of
> chunks is what we expect. The overall size is then necessarily
> correct.

Good point. I've removed the extra check on the total size, since it cannot be wrong if the checks on the individual chunk sizes were all correct. This eliminates the need for the odd calling convention for check_toast_tuple(), so I've changed that to return void and not take any return-by-reference arguments.

> - To me it would be more logical to reverse the order of the
> toast_pointer.va_toastrelid != ctx->rel->rd_rel->reltoastrelid and
> VARATT_EXTERNAL_GET_EXTSIZE(toast_pointer) > toast_pointer.va_rawsize
> - VARHDRSZ checks. Whether we're pointing at the correct relation
> feels more fundamental.

Done.

> - If we moved the toplevel foreach loop in check_toasted_attributes()
> out to the caller, say renaming the function to just
> check_toasted_attribute(), we'd save a level of indentation in that
> whole function and probably add a tad of clarity, too. You wouldn't
> feel the need to Assert(ctx.toasted_attributes == NIL) in the caller
> if the caller had just done list_free(ctx->toasted_attributes);
> ctx->toasted_attributes = NIL.

You're right. It looks nicer that way. Changed.

> - Why are all the changes to the tests in this patch? What do they
> have to do with getting the TOAST checks out from under the buffer
> lock? I really need you to structure the patch series so that each
> patch is about one topic and, equally, so that each topic is only
> covered by one patch. Otherwise it's just way too confusing.

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

v18-0002 - Postpones the toast checks for a page until after the main table page lock is released

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

v18-0004 - Adding corruption checks of toast pointers. Extends the regression tests to cover the new checks.

>
> - I think some of these messages need a bit of word-smithing, too, but
> we can leave that for when we're closer to being done with this.

Ok.