Re: WIP: Covering + unique indexes.

I mostly agree with your patch, nice work, but I have some notices for your patch:

1)
bt_target_page_check():
     if (!P_RIGHTMOST(topaque) &&
         !_bt_check_natts(state->rel, state->target, P_HIKEY))

Seems not very obvious: it looks like we don't need to check nattrs on rightmost 
page. Okay, I remember that on rightmost page there is no hikey at all, but at 
least comment should added. Implicitly bt_target_page_check() already takes into 
account 'is page rightmost or not?' by using  P_FIRSTDATAKEY, so, may be better 
to move rightmost check into bt_target_page_check() with some refactoring if-logic:

if (offset > maxoff)
	return true; //nothing to check, also covers empty rightmost page

if (P_ISLEAF) {
	if (offnum >= P_FIRSTDATAKEY)
		...
	else /* if (offnum == P_HIKEY) */
		...
}
else // !P_ISLEAF
{
	if (offnum == P_FIRSTDATAKEY)
		...
	else if (offnum > P_FIRSTDATAKEY)
		...
	else /* if (offnum == P_HIKEY) */
		...
}
	
I see it's possible only 3 nattrs value: 0, nkey and nkey+nincluded, but 
collapsing if-clause to three branches causes difficulties for code readers. Let 
compiler optimize that. Sorry for late notice, but it takes my attention only 
when I noticed (!P_RIGHTMOST && !_bt_check_natts) condition.

2)
Style notice:
         ItemPointerSetInvalid(&trunctuple.t_tid);
+   BTreeTupSetNAtts(&trunctuple, 0);
     if (PageAddItem(page, (Item) &trunctuple, sizeof(IndexTupleData), P_HIKEY,
It's better to have blank line between BTreeTupSetNAtts() and if clause.

3) Naming BTreeTupGetNAtts/BTreeTupSetNAtts - several lines above we use full 
Tuple word in dowlink macroses, here we use just Tup. Seems, better to have 
Tuple in both cases. Or Tup, but still in both cases.

4) BTreeTupSetNAtts - seems, it's better to add check  of nattrs to fits  to 
BT_N_KEYS_OFFSET_MASK  mask, and it should not touch BT_RESERVED_OFFSET_MASK 
bits, now it will overwrite that bits.

Attached patch is rebased to current head and contains some comment improvement 
in index_truncate_tuple() - you save some amount of memory with TupleDescCopy() 
call but didn't explain why pfree is enough to free all allocated memory.




Peter Geoghegan wrote:
> On Tue, Apr 17, 2018 at 3:12 AM, Alexander Korotkov
> <a.korotkov@postgrespro.ru> wrote:
>> Hmm, what do you think about making BTreeTupGetNAtts() take tupledesc
>> argument, not relation>  It anyway doesn't need number of key attributes,
>> only total number of attributes.  Then _bt_isequal() would be able to use
>> BTreeTupGetNAtts().
> 
> That would make the BTreeTupGetNAtts() assertions quite a bit more
> verbose, since there is usually no existing tuple descriptor variable,
> but there is almost always a "rel" variable. The coverage within
> _bt_isequal() does not seem important, because we only use it with the
> page high key in rare cases, where _bt_moveright() will already have
> tested the highkey.
> 
>> I think it's completely OK to fix broken things when you've to touch
>> them.  Probably, Teodor would decide to make that by separate commit.
>> So, it's up to him.
> 
> You're right to say that this old negative infinity tuple code within
> _bt_insertonpg() is broken code, and not just dead code. The code
> doesn't just confuse things (e.g. see recent commit 2a67d644). It also
> seems like it could actually be harmful. This is code that could only
> ever corrupt your database.
> 
> I'm fine if Teodor wants to commit that change separately, of course.
> 

-- 
Teodor Sigaev                                   E-mail: teodor@sigaev.ru
                                                    WWW: http://www.sigaev.ru/