Re: 9.5rc1 brin_summarize_new_values

Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
> On Sat, Dec 26, 2015 at 7:10 AM, Jeff Janes <jeff(dot)janes(at)gmail(dot)com> wrote:
>> brin_summarize_new_values() does not check that it is called on the
>> correct type of index, nor does it check permissions.

> Yeah, it should have those sanity checks...

Yeah, that is absolutely a must-fix.

> What do you think about the attached?

Don't like that as-is, because dropping and re-acquiring the index lock
presents a race condition: the checks you made might not apply anymore.
Admittedly, the odds of a problem are very small, but it's still an
insecure coding pattern.

I hesitate to produce code without having consumed any caffeine yet,
but maybe we could do it like this:

heapoid = IndexGetRelation(indexoid, true);
if (OidIsValid(heapoid))
heapRel = heap_open(heapoid, ShareUpdateExclusiveLock);
else
heapRel = NULL;
indexRel = index_open(indexoid, ShareUpdateExclusiveLock);

// make index validity checks here

// complain if heapRel is NULL (shouldn't happen if it was an index)

Also, as far as what the checks are, I'm inclined to insist that only
the owner of the index can apply brin_summarize_new_values to it.
SELECT privilege should definitely *not* be enough to allow modifying
the index contents, not to mention holding ShareUpdateExclusiveLock
on the table for what might be a long time. Checking ownership is
comparable to the privileges required for VACUUM. (I see that we also
allow the database owner to VACUUM, but I'm not sure on the use-case
for allowing that exception for brin_summarize_new_values.)

regards, tom lane