Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index.

From: Pavel Borisov <pashkin(dot)elfe(at)gmail(dot)com>
To: Peter Geoghegan <pg(at)bowt(dot)ie>
Cc: Noah Misch <noah(at)leadboat(dot)com>, Alexander Korotkov <aekorotkov(at)gmail(dot)com>, Aleksander Alekseev <aleksander(at)timescale(dot)com>, Postgres hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Maxim Orlov <orlovmg(at)gmail(dot)com>, Andres Freund <andres(at)anarazel(dot)de>, Greg Stark <stark(at)mit(dot)edu>, Julien Rouhaud <rjuju123(at)gmail(dot)com>, Mark Dilger <mark(dot)dilger(at)enterprisedb(dot)com>, David Steele <david(at)pgmasters(dot)net>, Maxim Orlov <m(dot)orlov(at)postgrespro(dot)ru>, lubennikovaav(at)gmail(dot)com, Hamid Akhtar <hamid(dot)akhtar(at)percona(dot)com>
Subject: Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index.
Date: 2024-04-17 15:41:10
Message-ID: CALT9ZEH4q6_J16rSK+goSwK+6JzX9DO86WfA_THyYKxw9yakFA@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

I did notice (I meant to point out) that I have concerns about this
> part of the new uniqueness check code:
> "
> if (P_IGNORE(topaque) || !P_ISLEAF(topaque))
> break;
> "

My concern here is with the !P_ISLEAF(topaque) test -- it shouldn't be
> required

I agree. But I didn't see the need to check uniqueness constraints
violations in internal pages. Furthermore, it doesn't mean only a violation
of constraint, but a major index corruption. I agree that checking and
reporting this type of corruption separately is a possible thing.

Separately, I dislike the way the target block changes within
> bt_target_page_check(). The general idea behind verify_nbtree.c's
> target block is that every block becomes the target exactly once, in a
> clearly defined place. All corruption (in the index structure itself)
> is formally considered to be a problem with that particular target
> block. I want to be able to clearly distinguish between the target and
> target's right sibling here, to explain my concerns, but they're kinda
> both the target, so that's a lot harder than it should be. (Admittedly
> directly blaming the target block has always been a little bit
> arbitrary, at least in certain cases, but even there it provides
> structure that makes things much easier to describe unambiguously.)
>

The possible way to load the target block only once is to get rid of the
cross-page uniqueness violation check. I introduced it to catch more
possible cases of uniqueness violations. Though they are expected to be
extremely rare, and anyway the algorithm doesn't get any warranty, just
does its best to catch what is possible. I don't object to this change.

Regards,
Pavel.

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2024-04-17 16:15:00 Re: ecpg_config.h symbol missing with meson
Previous Message Robert Haas 2024-04-17 15:03:55 Re: pg_combinebackup does not detect missing files