Re: 9.5rc1 brin_summarize_new_values

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
Cc: Jeff Janes <jeff(dot)janes(at)gmail(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: 9.5rc1 brin_summarize_new_values
Date: 2015-12-26 16:27:20
Message-ID: 23826.1451147240@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Teodor Sigaev 2015-12-26 18:04:58 POC, WIP: OR-clause support for indexes
Previous Message Tom Lane 2015-12-26 16:06:37 Re: Performance degradation in commit ac1d794