pg.ddx.io  pgsql-hackers@postgresql.org mailing list archive  

help / color / mirror / Atom feed
From: Mark Dilger <mark.dilger@enterprisedb.com>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Amul Sul <sulamul@gmail.com>
Cc: Dilip Kumar <dilipbalaut@gmail.com>
Cc: Peter Geoghegan <pg@bowt.ie>
Cc: Andres Freund <andres@anarazel.de>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: new heapcheck contrib module
Date: Sun, 2 Aug 2020 20:17:25 -0700
Message-ID: <53B5B281-81BF-40FD-9903-EBAA5193506B@enterprisedb.com> (raw)
In-Reply-To: <CA+TgmoZT3kCOaKq6=m5wHGKh_9XQDbCC1LZXL6w_+NMB-n1+mw@mail.gmail.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>
	<CAFiTN-uD3WSeggU20yU6voGou6BDb-cQcfkE8B1rAzWKZVK-Bw@mail.gmail.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>



> On Jul 30, 2020, at 10:59 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> 
> + curchunk = DatumGetInt32(fastgetattr(toasttup, 2,
> + ctx->toast_rel->rd_att, &isnull));
> 
> Should we be worrying about the possibility of fastgetattr crapping
> out if the TOAST tuple is corrupted?

I think we should, but I'm not sure we should be worrying about it at this location.  If the toast index is corrupt, systable_getnext_ordered could trip over the index corruption in the process of retrieving the toast tuple, so checking the toast tuple only helps if the toast index does not cause a crash first.  I think the toast index should be checked before this point, ala verify_nbtree, so that we don't need to worry about that here.  It might also make more sense to verify the toast table ala verify_heapam prior to here, so we don't have to worry about that here either.  But that raises questions about whose responsibility this all is.  If verify_heapam checks the toast table and toast index before the main table, that takes care of it, but makes a mess of the idea of verify_heapam taking a start and end block, since verifying the toast index is an all or nothing proposition, not something to be done in incremental pieces.  If we leave verify_heapam as it is, then it is up to the caller to check the toast before the main relation, which is more flexible, but is more complicated and requires the user to remember to do it.  We could split the difference by having verify_heapam do nothing about toast, leaving it up to the caller, but make pg_amcheck handle it by default, making it easier for users to not think about the issue.  Users who want to do incremental checking could still keep track of the chunks that have already been checked, not just for the main relation, but for the toast relation, too, and give start and end block arguments to verify_heapam for the toast table check and then again for the main table check.  That doesn't fix the question of incrementally checking the index, though.

Looking at it a slightly different way, I think what is being checked at the point in the code you mention is the logical structure of the toasted value related to the current main table tuple, not the lower level tuple structure of the toast table.  We already have a function for checking a heap, namely verify_heapam, and we (or the caller, really) should be using that.  The clean way to do things is

	verify_heapam(toast_rel)
	verify_btreeam(toast_idx)
	verify_heapam(main_rel)

and then depending on how fast and loose you want to be, you can use the start and end block arguments, which are inherently a bit half-baked, given the lack of any way to be sure you check precisely the right range of blocks, and also you can be fast and loose about skipping the index check or not, as you see fit.

Thoughts?

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company








view thread (218+ messages)  latest in thread

Message-ID: <53B5B281-81BF-40FD-9903-EBAA5193506B@enterprisedb.com>
Permalink:  ../53B5B281-81BF-40FD-9903-EBAA5193506B@enterprisedb.com/
Also on:    postgresql.org/message-id/53B5B281-81BF-40FD-9903-EBAA5193506B@enterprisedb.com

 · 

reply
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, sulamul@gmail.com, dilipbalaut@gmail.com, pg@bowt.ie, andres@anarazel.de
  Subject: Re: new heapcheck contrib module
  In-Reply-To: <53B5B281-81BF-40FD-9903-EBAA5193506B@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