public inbox for [email protected]  

help / color / mirror / Atom feed

From: Mark Dilger <[email protected]>
To: Pavel Borisov <[email protected]>
Cc: Anastasia Lubennikova <[email protected]>
Cc: Postgres hackers <[email protected]>
Cc: Peter Geoghegan <[email protected]>
Subject: Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index.
Date: Mon, 1 Mar 2021 13:05:03 -0800
Message-ID: <[email protected]> (raw)
In-Reply-To: <CALT9ZEGgKAXDfVaAjBjBf4e6-L_4NVQsJ+7gzHd0JLwNxjXTFA@mail.gmail.com>
References: <CALT9ZEHRn5xAM5boga0qnrCmPV52bScEK2QnQ1HmUZDD301JEg@mail.gmail.com>
	<[email protected]>
	<CALT9ZEFYsYPms0SPp4ZxpFCOBQbgE+iopoVKzU8Pni8Y6+RKtQ@mail.gmail.com>
	<[email protected]>
	<CALT9ZEGgKAXDfVaAjBjBf4e6-L_4NVQsJ+7gzHd0JLwNxjXTFA@mail.gmail.com>



> On Mar 1, 2021, at 12:23 PM, Pavel Borisov <[email protected]> wrote:
> 
> If bt_index_check() and bt_index_parent_check() are to have this functionality, shouldn't there be an option controlling it much as the option (heapallindexed boolean) controls checking whether all entries in the heap are indexed in the btree?  It seems inconsistent to have an option to avoid checking the heap for that, but not for this.  Alternately, this might even be better coded as its own function, named something like bt_unique_index_check() perhaps.  I hope Peter might advise?
>  
> As for heap checking, my reasoning was that we can not check whether a unique constraint violated by the index, without checking heap tuple visibility. I.e. we can have many identical index entries without uniqueness violated if only one of them corresponds to a visible heap tuple. So heap checking included in my patch is _necessary_ for unique constraint checking, it should not have an option to be disabled, otherwise, the only answer we can get is that unique constraint MAY be violated and may not be, which is quite useless. If you meant something different, please elaborate. 

I completely agree that checking uniqueness requires looking at the heap, but I don't agree that every caller of bt_index_check on an index wants that particular check to be performed.  There are multiple ways in which an index might be corrupt, and Peter wrote the code to only check some of them by default, with options to expand the checks to other things.  This is why heapallindexed is optional.  If you don't want to pay the price of checking all entries in the heap against the btree, you don't have to.

I'm not against running uniqueness checks on unique indexes.  It seems fairly normal that a user would want that.  Perhaps the option should default to 'true' if unspecified?  But having no option at all seems to run contrary to how the other functionality is structured.

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

view thread (32+ messages)  latest in thread

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: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index.
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox