Re: WIP: Covering + unique indexes.

On 2017-03-30 18:26:05 +0300, Teodor Sigaev wrote:
> Any objection from reviewers to push both patches?


> 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.


> +      <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.


> +/*
> + * 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.


Maybe also rename the function to index_build_key_tuple()?

>   * Construct a string describing the contents of an index entry, in the
>   * form "(key_name, ...)=(key_value, ...)".  This is currently used
> - * for building unique-constraint and exclusion-constraint error messages.
> + * for building unique-constraint and exclusion-constraint error messages,
> + * so only key columns of index are checked and printed.

s/index/the index/


> @@ -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?


> --- 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.


> @@ -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.



> @@ -340,14 +341,27 @@ DefineIndex(Oid relationId,
>  	numberOfAttributes = list_length(stmt->indexParams);
> -	if (numberOfAttributes <= 0)
> -		ereport(ERROR,
> -				(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
> -				 errmsg("must specify at least one column")));
> +

Huh, why's that check gone?

>  
> +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?


> @@ -1979,6 +2017,48 @@ transformIndexConstraint(Constraint *constraint, CreateStmtContext *cxt)
>  		index->indexParams = lappend(index->indexParams, iparam);
>  	}
>  
> +	/* Here is some code duplication. But we do need it. */

Aha?


> +	foreach(lc, constraint->including)
> +	{
> +		char	   *key = strVal(lfirst(lc));
> +		bool		found = false;
> +		ColumnDef  *column = NULL;
> +		ListCell   *columns;
> +		IndexElem  *iparam;
> +
> +		foreach(columns, cxt->columns)
> +		{
> +			column = (ColumnDef *) lfirst(columns);
> +			Assert(IsA(column, ColumnDef));
> +			if (strcmp(column->colname, key) == 0)
> +			{
> +				found = true;
> +				break;
> +			}
> +		}
> +
> +		/*
> +		 * In the ALTER TABLE case, don't complain about index keys not
> +		 * created in the command; they may well exist already. DefineIndex
> +		 * will complain about them if not, and will also take care of marking
> +		 * them NOT NULL.
> +		 */

Uh. Why should they be marked as NOT NULL? ISTM the comment has been
copied here without adjustments.



> @@ -1275,6 +1275,21 @@ pg_get_indexdef_worker(Oid indexrelid, int colno,
>  		Oid			keycoltype;
>  		Oid			keycolcollation;
>  
> +		/*
> +		 * attrsOnly flag is used for building unique-constraint and
> +		 * exclusion-constraint error messages. Included attrs are
> +		 * meaningless there, so do not include them in the message.
> +		 */
> +		if (attrsOnly && keyno >= idxrec->indnkeyatts)
> +			break;

Sounds like the parameter should be renamed then.



> +Included attributes in B-tree indexes
> +-------------------------------------
> +
> +Since 10.0 there is an optional INCLUDE clause, that allows to add

10.0 isn't right, since that's the "patch" version now.


> +a portion of non-key attributes to index. They exist to allow more queries
> +to benefit from index-only scans. We never use included attributes in
> +ScanKeys, neither for search nor for inserts. That allows us to include
> +into B-tree any datatypes, even those which don't have suitable opclass.
> +Included columns only stored in regular items on leaf pages. All inner
> +keys and high keys are truncated and contain only key attributes.
> +That helps to reduce the size of index.

s/index/the index/



> @@ -537,6 +542,28 @@ _bt_buildadd(BTWriteState *wstate, BTPageState *state, IndexTuple itup)
>  		ItemIdSetUnused(ii);	/* redundant */
>  		((PageHeader) opage)->pd_lower -= sizeof(ItemIdData);
>  
> +		if (indnkeyatts != indnatts && P_ISLEAF(opageop))
> +		{
> +			/*
> +			 * It's essential to truncate High key here.
> +			 * The purpose is not just to save more space on this particular page,
> +			 * but to keep whole b-tree structure consistent. Subsequent insertions
> +			 * assume that hikey is already truncated, and so they should not
> +			 * worry about it, when copying the high key into the parent page
> +			 * as a downlink.

s/should/need/

> +			 * NOTE It is not crutial for reliability in present,

s/crutial/crucial/

> +			 * but maybe it will be that in the future.
> +			 */

"it's essential" ... "it is not crutial" -- that's contradictory.

> +			keytup = index_truncate_tuple(wstate->index, oitup);

The code in _bt_split previously claimed that it's the only place doing
truncation...


> +			/*  delete "wrong" high key, insert keytup as P_HIKEY. */
> +			PageIndexTupleDelete(opage, P_HIKEY);

> +			if (!_bt_pgaddtup(opage, IndexTupleSize(keytup), keytup, P_HIKEY))
> +				elog(ERROR, "failed to rewrite compressed item in index \"%s\"",
> +					RelationGetRelationName(wstate->index));

Hm...


- Andres