On Tue, Apr 10, 2018 at 5:45 PM, Peter Geoghegan <p...@bowt.ie> wrote: > I did find another problem, though. Looks like the idea to explicitly > represent the number of attributes directly has paid off already: > > pg@~[3711]=# create table covering_bug (f1 int, f2 int, f3 text); > create unique index cov_idx on covering_bug (f1) include(f2); > insert into covering_bug select i, i * random() * 1000, i * random() * > 100000 from generate_series(0,100000) i; > DEBUG: building index "pg_toast_16451_index" on table "pg_toast_16451" > serially > CREATE TABLE > DEBUG: building index "cov_idx" on table "covering_bug" serially > CREATE INDEX > ERROR: tuple has wrong number of attributes in index "cov_idx"
Actually, this was an error on my part (though I'd still maintain that the check paid off here!). I'll still add defensive assertions inside _bt_newroot(), and anywhere else that they're needed. There is no reason to not add defensive assertions in all code that handles page splits, and needs to fetch a highkey from some other page. We missed a few of those. I'll add an item to "Decisions to Recheck Mid-Beta" section of the open items page for this patch. We should review the decision to make a call to _bt_check_natts() within _bt_compare(). It might work just as well as an assertion, and it would be unfortunate if workloads that don't use covering indexes had to pay a price for the _bt_check_natts() call, even if it was a small price. I've seen _bt_compare() appear prominently in profiles quite a few times. -- Peter Geoghegan
