Re: pgsql: Skip full index scan during cleanup of B-tree indexes when possi

On Thu, Apr 5, 2018 at 3:24 PM, Alexander Korotkov <
a(dot)korotkov(at)postgrespro(dot)ru> wrote:

> On Thu, Apr 5, 2018 at 2:26 PM, Alexander Korotkov <
> a(dot)korotkov(at)postgrespro(dot)ru> wrote:
>
>> On Thu, Apr 5, 2018 at 6:28 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>
>>> Peter Geoghegan <pg(at)bowt(dot)ie> writes:
>>> >>> TRAP: FailedAssertion("!(metad->btm_version == 3)", File:
>>> >>> "/home/pg/postgresql/root/build/../source/src/backend/access
>>> /nbtree/nbtpage.c",
>>> >>> Line: 619)
>>>
>>> >> Hm, buildfarm's not complaining --- what's the test case?
>>>
>>> > This was discovered while testing/reviewing the latest version of the
>>> > INCLUDE covering indexes patch. It now seems to be unrelated.
>>>
>>> Oh, wait ... I wonder if you saw that because you were running a new
>>> backend without having re-initdb'd? Once you had re-initdb'd, then
>>> of course there would be no old-format btree indexes anywhere. But
>>> if you hadn't, then anyplace that was not prepared to cope with the
>>> old header format would complain about pre-existing indexes.
>>>
>>> In short, this sounds like a place that did not get the memo about
>>> how to cope with un-upgraded indexes.
>>>
>>
>> That's an issue, because meta-page should be upgraded "on the fly".
>> That was tested, but perhaps without assertions. I'll investigate more on
>> this an propose a fix.
>>
>
> So, "Assert(metad->btm_version == BTREE_VERSION)" was failed, because
> metadata was copied from meta page to rel->rd_amcache "as is" with
> metad->btm_version == BTREE_MIN_VERSION.
>
> Without assert everything works fine, because extended metadata fields are
> never used from rel->rd_amcache. So my first intention was to relax this
> assert to Assert(metad->btm_version >= BTREE_MIN_VERSION && metad->btm_version
> <= BTREE_VERSION).
> But then I still have concern that we copy metadata beyond pd_lower
> when metapage is in old format.
>
> memcpy(rel->rd_amcache, metad, sizeof(BTMetaPageData));
>
> This is why I decided to write separate function to handle caching of
> metadata,
> which takes care about filling unavailable fields of metadata with default
> values.
> I also made same fix for pageinspect. Patch is attached.
>

I forgot to note, that I've tested this patch in following way. I did
initdb and
created some tables and indexes on eac93e20af. And then used the
same cluster on patched master including index scans, some
inserts/updates/deletes,
bt_metap(), vacuum and so on. Everything worked fine.
Sorry, that I didn't test that enough before.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company