Subject: Re: [HACKERS] Surjective functional indexes
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: Stephen Frost <sfrost@snowman.net>,
 Michael Paquier <michael.paquier@gmail.com>,
 Simon Riggs <simon@2ndquadrant.com>, Christoph Berg <myon@debian.org>,
 Thomas Munro <thomas.munro@enterprisedb.com>,
 Peter Eisentraut <peter.eisentraut@2ndquadrant.com>,
 PostgreSQL Hackers <pgsql-hackers@postgresql.org>
References: <20180322205353.5v6g5zu3owzh43qp@alvherre.pgsql>
From: Konstantin Knizhnik <k.knizhnik@postgrespro.ru>
Message-ID: <0f342b0f-3bce-61d2-a411-1e5e15036f0b@postgrespro.ru>
Date: Fri, 23 Mar 2018 19:08:59 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.4.0
MIME-Version: 1.0
In-Reply-To: <20180322205353.5v6g5zu3owzh43qp@alvherre.pgsql>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Precedence: bulk


On 22.03.2018 23:53, Alvaro Herrera wrote:
> The whole IsProjectionFunctionalIndex looks kinda bogus/ugly to me.  Set
> the boolean to false, but keep evaluating anyway?  But then, I thought
> the idea was to do this based on the reloption, not by comparing the
> expression cost to a magical (unmodifiable) value?
The idea is very simple, I tried to explain it in the comments. Sorry if 
my explanation was not so clear.
By default we assume all functional indexes as projectional. This is 
done by first assignment is_projection = true.
Then we check cost of index function.
I agree that "magical (unmodifiable) value" used as cost threshold is 
not a good idea. But I tried to avoid as much as possible adding yet 
another configuration parameter.
I do no think that flexibility here is so important. I believe that in 
99% cases functional indexes are projectional,
and in all other cases DBA cna explicitly specify it using 
recheck_on_update index option.
Which should override cost threshold: if DBA thinks that recheck is 
useful for this index, then it should be used despite to previsions 
guess based on index expression cost.
This is why after setting "is_projection" to false because of too large 
cost of index expression, we  continue work and check index options for 
explicitly specified value.

>
> In RelationGetIndexAttrBitmap(), indexattrs no longer gets the columns
> corresponding to projection indexes.  Isn't that weird/error
> prone/confusing?  I think it'd be saner to add these bits to both
> bitmaps.
This bitmap is needed only to mark attributes, which modification 
prevents hot update.
Should I rename rd_indexattr to  rd_hotindexattr or whatever else to 
avoid confusion?
Please notice, that rd_indexattr is used only inside 
RelationGetIndexAttrBitmap and is not visible from outside.
It is returned for INDEX_ATTR_BITMAP_HOT IndexAttrBitmapKind.

> Please update the comments ending in heapam.c:4188, and generally all
> comments that you should update.
Sorry, did you apply projection-7.patch?
I have checked all trailing spaces and other indentation issues.
At least there is no trailing space at heapam.c:4188...

>
> Please keep serial_schedule in sync with parallel_schedule.

Sorry, once again do not understand your complaint: I have added 
func_index both to serial and parallel schedule.

> Also, pgindent.
>

-- 
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company