Обсуждение: 9.5rc1 brin_summarize_new_values
brin_summarize_new_values() does not check that it is called on the correct type of index, nor does it check permissions.
On Sat, Dec 26, 2015 at 7:10 AM, Jeff Janes <jeff.janes@gmail.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... On top of that this is not
a really user-friendly message:
=# select brin_summarize_new_values('brin_example'::regclass);
ERROR: XX000: cache lookup failed for index 16391
LOCATION: IndexGetRelation, index.c:3279
What do you think about the attached? This should definitely be fixed
by 9.5.0...
--
Michael
Вложения
Michael Paquier <michael.paquier@gmail.com> writes:
> On Sat, Dec 26, 2015 at 7:10 AM, Jeff Janes <jeff.janes@gmail.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
On Sat, Dec 26, 2015 at 8:27 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Michael Paquier <michael.paquier@gmail.com> writes: >> On Sat, Dec 26, 2015 at 7:10 AM, Jeff Janes <jeff.janes@gmail.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. Thanks for committing the fix. Another issue is: it seems to have no SGML documentation. Is that OK? Cheers, Jeff
Jeff Janes wrote: > On Sat, Dec 26, 2015 at 8:27 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Michael Paquier <michael.paquier@gmail.com> writes: > >> On Sat, Dec 26, 2015 at 7:10 AM, Jeff Janes <jeff.janes@gmail.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. > > Thanks for committing the fix. Yes, thanks. > Another issue is: it seems to have no SGML documentation. Is that OK? Hmm, I'm pretty sure I documented it somewhere. If this can wait till Monday, I'll have a look by then. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Sun, Dec 27, 2015 at 1:27 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Michael Paquier <michael.paquier@gmail.com> writes: >> On Sat, Dec 26, 2015 at 7:10 AM, Jeff Janes <jeff.janes@gmail.com> wrote: >> 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: > > [...] I see, thanks! The lesson is learnt. -- Michael
Jeff Janes wrote:
> Another issue is: it seems to have no SGML documentation. Is that OK?
Here's a patch (which I had to write afresh, because I couldn't find
anything in my brin development tree. Maybe I was just remembering
something I planned to do and never got around to.)
This creates a new sub-section at the bottom of "9.26 System
Administration Functions" named "Indexing Helper Functions", containing
a table with a single row which is for this function. Also, in the BRIN
chapter it creates a subsection "62.1.1. Index Maintenance" which has
one paragraph mentioning that pages that aren't already summarized are
only processed by VACUUM or this function.
I thought about moving the last paragraph of the introduction (which
talks about pages_per_range) to the new subsection. It's clearly of a
different spirit that the preceding paragraphs, but then that parameter
is not really "maintenance" of the index. Perhaps I should name the
subsection something like "Operation and Maintenance" instead, and then
those two paragraphs fit in there.
Other opinions?
Regarding 9.26, this is its TOC:
9.26. System Administration Functions
9.26.1. Configuration Settings Functions
9.26.2. Server Signaling Functions
9.26.3. Backup Control Functions
9.26.4. Recovery Control Functions
9.26.5. Snapshot Synchronization Functions
9.26.6. Replication Functions
9.26.7. Database Object Management Functions
9.26.8. Generic File Access Functions
9.26.9. Advisory Lock Functions
9.26.10. Indexing Helper Functions
We can bikeshed whether the new subsection should be at the bottom, or
some other placement. Maybe it should become 9.26.3, for example.
--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Вложения
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> This creates a new sub-section at the bottom of "9.26 System
> Administration Functions" named "Indexing Helper Functions", containing
> a table with a single row which is for this function.
Perhaps call it "Index Maintenance Functions"?
> We can bikeshed whether the new subsection should be at the bottom, or
> some other placement. Maybe it should become 9.26.3, for example.
Perhaps right after Database Object Management Functions. I'm not
feeling especially picky about that though; bottom would be OK.
regards, tom lane
Tom Lane wrote: > Alvaro Herrera <alvherre@2ndquadrant.com> writes: > > This creates a new sub-section at the bottom of "9.26 System > > Administration Functions" named "Indexing Helper Functions", containing > > a table with a single row which is for this function. > > Perhaps call it "Index Maintenance Functions"? > > > We can bikeshed whether the new subsection should be at the bottom, or > > some other placement. Maybe it should become 9.26.3, for example. > > Perhaps right after Database Object Management Functions. I'm not > feeling especially picky about that though; bottom would be OK. Picked up both suggestions (along with some additional rewording and fixes to the sgml tags), and pushed. Thanks. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services