Re: WIP: Covering + unique indexes.

30.03.2017 22:11, Andres Freund
> Any objection from reviewers to push both patches?

First of all, I want to thank you and Robert for reviewing this patch.
Your expertise in postgres subsystems is really necessary for features 
like this.
I just wonder, why don't you share your thoughts and doubts till the 
"last call".

>> diff --git a/contrib/bloom/blutils.c b/contrib/bloom/blutils.c
>> index f2eda67..59029b9 100644
>> --- a/contrib/bloom/blutils.c
>> +++ b/contrib/bloom/blutils.c
>> @@ -120,6 +120,7 @@ blhandler(PG_FUNCTION_ARGS)
>>   	amroutine->amclusterable = false;
>>   	amroutine->ampredlocks = false;
>>   	amroutine->amcanparallel = false;
>> +	amroutine->amcaninclude = false;
> That name doesn't strike me as very descriptive.
>
The feature is "index with included columns", it uses keyword "INCLUDE".
So the name looks good to me.
Any suggestions?
>> +      <term><literal>INCLUDE</literal></term>
>> +      <listitem>
>> +       <para>
>> +        An optional <literal>INCLUDE</> clause allows a list of columns to be
>> +        specified which will be included in the non-key portion of the index.
>> +        Columns which are part of this clause cannot also exist in the
>> +        key columns portion of the index, and vice versa. The
>> +        <literal>INCLUDE</> columns exist solely to allow more queries to benefit
>> +        from <firstterm>index-only scans</> by including certain columns in the
>> +        index, the value of which would otherwise have to be obtained by reading
>> +        the table's heap. Having these columns in the <literal>INCLUDE</> clause
>> +        in some cases allows <productname>PostgreSQL</> to skip the heap read
>> +        completely. This also allows <literal>UNIQUE</> indexes to be defined on
>> +        one set of columns, which can include another set of columns in the
>> +       <literal>INCLUDE</> clause, on which the uniqueness is not enforced.
>> +        It's the same with other constraints (PRIMARY KEY and EXCLUDE). This can
>> +        also can be used for non-unique indexes as any columns which are not required
>> +        for the searching or ordering of records can be used in the
>> +        <literal>INCLUDE</> clause, which can slightly reduce the size of the index.
>> +        Currently, only the B-tree access method supports this feature.
>> +        Expressions as included columns are not supported since they cannot be used
>> +        in index-only scans.
>> +       </para>
>> +      </listitem>
>> +     </varlistentry>
> This could use some polishing.
Definitely. But do you have any specific proposals?

>> +/*
>> + * Reform index tuple. Truncate nonkey (INCLUDE) attributes.
>> + */
>> +IndexTuple
>> +index_truncate_tuple(Relation idxrel, IndexTuple olditup)
>> +{
>> +	TupleDesc   itupdesc = RelationGetDescr(idxrel);
>> +	Datum       values[INDEX_MAX_KEYS];
>> +	bool        isnull[INDEX_MAX_KEYS];
>> +	IndexTuple	newitup;
>> +	int indnatts = IndexRelationGetNumberOfAttributes(idxrel);
>> +	int indnkeyatts = IndexRelationGetNumberOfKeyAttributes(idxrel);
>> +
>> +	Assert(indnatts <= INDEX_MAX_KEYS);
>> +	Assert(indnkeyatts > 0);
>> +	Assert(indnkeyatts < indnatts);
>> +
>> +	index_deform_tuple(olditup, itupdesc, values, isnull);
>> +
>> +	/* form new tuple that will contain only key attributes */
>> +	itupdesc->natts = indnkeyatts;
>> +	newitup = index_form_tuple(itupdesc, values, isnull);
>> +	newitup->t_tid = olditup->t_tid;
>> +
>> +	itupdesc->natts = indnatts;
> Uh, isn't this a *seriously* bad idea?  If index_form_tuple errors out,
> this'll corrupt the tuple descriptor.
Initial reasoning was something like this:

> Maybe it would be better to modify index_form_tuple() to accept a new
> argument with a number of attributes, then you can just Assert that
> this number is never higher than the number of attributes in the
> TupleDesc.
Good point.
I agree that this function is a bit strange. I have to set
tupdesc->nattrs to support compatibility with index_form_tuple().
I didn't want to add neither a new field to tupledesc nor a new
parameter to index_form_tuple(), because they are used widely.


But I haven't considered the possibility of index_form_tuple() failure.
Fixed in this version of the patch. Now it creates a copy of tupledesc 
to pass it to index_form_tuple.

> Maybe also rename the function to index_build_key_tuple()?
We'd discussed with other reviewers, they suggested 
index_truncate_tuple() instead of index_reform_tuple().
I think that this name reflects the essence of the function clear enough 
and don't feel like renaming it again.

>> @@ -368,7 +370,7 @@ systable_beginscan(Relation heapRelation,
>>   		{
>>   			int			j;
>>   
>> -			for (j = 0; j < irel->rd_index->indnatts; j++)
>> +			for (j = 0; j < IndexRelationGetNumberOfAttributes(irel); j++)
>>   			{
>>   				if (key[i].sk_attno == irel->rd_index->indkey.values[j])
>>   				{
>> @@ -376,7 +378,7 @@ systable_beginscan(Relation heapRelation,
>>   					break;
>>   				}
>>   			}
>> -			if (j == irel->rd_index->indnatts)
>> +			if (j == IndexRelationGetNumberOfAttributes(irel))
>>   				elog(ERROR, "column is not in index");
>>   		}
> Not that it matters overly much, but why are we doing this for all
> attributes, rather than just key attributes?
Since we don't use included columns for system indexes, there is no 
difference. I've just tried to minimize code changes here.
>> --- a/src/backend/bootstrap/bootstrap.c
>> +++ b/src/backend/bootstrap/bootstrap.c
>> @@ -600,7 +600,7 @@ boot_openrel(char *relname)
>>   		 relname, (int) ATTRIBUTE_FIXED_PART_SIZE);
>>   
>>   	boot_reldesc = heap_openrv(makeRangeVar(NULL, relname, -1), NoLock);
>> -	numattr = boot_reldesc->rd_rel->relnatts;
>> +	numattr = RelationGetNumberOfAttributes(boot_reldesc);
>>   	for (i = 0; i < numattr; i++)
>>   	{
>>   		if (attrtypes[i] == NULL)
> That seems a bit unrelated.
I've replaced all the references to relnatts with macro, primarily to 
ensure that I won't miss anything that should use only key attributes.
>> @@ -2086,7 +2086,8 @@ StoreRelCheck(Relation rel, char *ccname, Node *expr,
>>   							  is_validated,
>>   							  RelationGetRelid(rel),	/* relation */
>>   							  attNos,	/* attrs in the constraint */
>> -							  keycount, /* # attrs in the constraint */
>> +							  keycount, /* # key attrs in the constraint */
>> +							  keycount, /* # total attrs in the constraint */
>>   							  InvalidOid,		/* not a domain constraint */
>>   							  InvalidOid,		/* no associated index */
>>   							  InvalidOid,		/* Foreign key fields */
> It doesn't quite seem right to me to store this both in pg_index and
> pg_constraint.
>
Initially, I did to provide pg_get_constraintdef_worker() with info 
about included columns.
Maybe it can be solved in some other way, but for now it is a tested and 
working implementation.

>>   
>> +opt_c_include:	INCLUDE '(' columnList ')'			{ $$ = $3; }
>> +			 |		/* EMPTY */						{ $$ = NIL; }
>> +		;
>> +opt_include:		INCLUDE '(' index_including_params ')'			{ $$ = $3; }
>> +			 |		/* EMPTY */						{ $$ = NIL; }
>> +		;
>> +
>> +index_including_params:	index_elem						{ $$ = list_make1($1); }
>> +			| index_including_params ',' index_elem		{ $$ = lappend($1, $3); }
>> +		;
>> +
> Why do we have multiple different definitions of this?

Hm,
columnList contains entries of columnElem type and 
index_including_params works with index_elem.
Is there a way they can be combined?

>> +			keytup = index_truncate_tuple(wstate->index, oitup);
> The code in _bt_split previously claimed that it's the only place doing
> truncation...
To be exact, it claimed that regarding insertion of new values, not 
index build.
 > It's the only point in insertion process, where we perform truncation


Other comments about code format, spelling and comments are fixed in 
attached patches.
Thank you again for reviewing.

-- 
Anastasia Lubennikova
Postgres Professional:http://www.postgrespro.com
The Russian Postgres Company