Re: [HACKERS] 9.5rc1 brin_summarize_new_values
Tom Lane wrote: > Alvaro Herrera 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 Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] 9.5rc1 brin_summarize_new_values
Alvaro Herrera 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] 9.5rc1 brin_summarize_new_values
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 Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services diff --git a/doc/src/sgml/brin.sgml b/doc/src/sgml/brin.sgml index 2202b7a..c1b6e1a 100644 --- a/doc/src/sgml/brin.sgml +++ b/doc/src/sgml/brin.sgml @@ -58,6 +58,24 @@ store more index entries), but at the same time the summary data stored can be more precise and more data blocks can be skipped during an index scan. + + + Index Maintenance + + + At the time of creation, all existing index pages are scanned and a + summary is created for each range, including the possibly-incomplete + range at the end. As new pages are filled with data, page ranges that + are already summarized will cause the summary information to be updated + with the new tuples. When a new page is created that does not fall + into the last summarized range, that range does not automatically + acquire a summary tuple; those insertions remain unsummarized until + a summarization run is invoked later, which creates initial summaries + for all unsummarized ranges. This process can be invoked manually + by the brin_summarize_new_pages(regclass) function, + or automatically when VACUUM processes the table. + + diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml index 81c1d3f..577d3bd 100644 --- a/doc/src/sgml/func.sgml +++ b/doc/src/sgml/func.sgml @@ -18281,6 +18281,48 @@ SELECT (pg_stat_file('filename')).modification; + + Indexing Helper Functions + + +brin_summarize_new_values + + + + shows the functions +available for index maintenance tasks. + + + +Indexing Helper Functions + + + Name Return Type Description + + + + + +brin_summarize_new_values(index_oid) + + integer + summarize page ranges not already summarized + + + + + + +brin_summarize_new_values receives a BRIN index OID as +argument; it scans the table that the index is for, looking for pages +that are not currently summarized. Then the data in those pages is +scanned and a new summary index tuple is constructed and inserted into +the index. It returns the number of new page range summaries that were +inserted into the index. + + + + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] 9.5rc1 brin_summarize_new_values
On Sun, Dec 27, 2015 at 1:27 AM, Tom Lane wrote: > Michael Paquier writes: >> On Sat, Dec 26, 2015 at 7:10 AM, Jeff Janes 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] 9.5rc1 brin_summarize_new_values
Jeff Janes wrote: > On Sat, Dec 26, 2015 at 8:27 AM, Tom Lane wrote: > > Michael Paquier writes: > >> On Sat, Dec 26, 2015 at 7:10 AM, Jeff Janes 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 Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] 9.5rc1 brin_summarize_new_values
On Sat, Dec 26, 2015 at 8:27 AM, Tom Lane wrote: > Michael Paquier writes: >> On Sat, Dec 26, 2015 at 7:10 AM, Jeff Janes 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] 9.5rc1 brin_summarize_new_values
Michael Paquier writes: > On Sat, Dec 26, 2015 at 7:10 AM, Jeff Janes 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] 9.5rc1 brin_summarize_new_values
On Sat, Dec 26, 2015 at 7:10 AM, Jeff Janes 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 diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c index 99337b0..19478d1 100644 --- a/src/backend/access/brin/brin.c +++ b/src/backend/access/brin/brin.c @@ -789,8 +789,30 @@ brin_summarize_new_values(PG_FUNCTION_ARGS) Oid indexoid = PG_GETARG_OID(0); Relation indexRel; Relation heapRel; + Relation relation; double numSummarized = 0; + relation = relation_open(indexoid, ShareUpdateExclusiveLock); + if (relation->rd_rel->relkind != RELKIND_INDEX) + ereport(ERROR, +(errcode(ERRCODE_WRONG_OBJECT_TYPE), + errmsg("\"%s\" is not an index", + RelationGetRelationName(relation; + + if (relation->rd_rel->relam != BRIN_AM_OID) + ereport(ERROR, +(errcode(ERRCODE_WRONG_OBJECT_TYPE), + errmsg("\"%s\" is not a BRIN index", + RelationGetRelationName(relation; + + if (pg_class_aclcheck(indexoid, GetUserId(), ACL_SELECT) != ACLCHECK_OK) + ereport(ERROR, +(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("permission denied for index %s", + RelationGetRelationName(relation; + + relation_close(relation, ShareUpdateExclusiveLock); + heapRel = heap_open(IndexGetRelation(indexoid, false), ShareUpdateExclusiveLock); indexRel = index_open(indexoid, ShareUpdateExclusiveLock); diff --git a/src/test/regress/expected/brin.out b/src/test/regress/expected/brin.out index 0937b63..9dd5e6e 100644 --- a/src/test/regress/expected/brin.out +++ b/src/test/regress/expected/brin.out @@ -407,3 +407,26 @@ FROM tenk1 ORDER BY unique2 LIMIT 5 OFFSET 5; VACUUM brintest; -- force a summarization cycle in brinidx UPDATE brintest SET int8col = int8col * int4col; UPDATE brintest SET textcol = '' WHERE textcol IS NOT NULL; +-- Tests for brin_summarize_new_values +CREATE TABLE brin_example (id int); +CREATE INDEX btree_index ON brin_example(id); +CREATE INDEX brin_index ON brin_example USING brin(id); +-- Checks on relation type +SELECT brin_summarize_new_values('brin_example'::regclass); -- error +ERROR: "brin_example" is not an index +SELECT brin_summarize_new_values('btree_index'::regclass); -- error +ERROR: "btree_index" is not a BRIN index +SELECT brin_summarize_new_values('brin_index'::regclass); -- ok + brin_summarize_new_values +--- + 0 +(1 row) + +-- Permission checks +CREATE ROLE brin_role; +SET SESSION AUTHORIZATION brin_role; +SELECT brin_summarize_new_values('brin_index'::regclass); -- error +ERROR: permission denied for index brin_index +RESET SESSION AUTHORIZATION; +DROP ROLE brin_role; +DROP TABLE brin_example; diff --git a/src/test/regress/sql/brin.sql b/src/test/regress/sql/brin.sql index db78d05..69a3ba8 100644 --- a/src/test/regress/sql/brin.sql +++ b/src/test/regress/sql/brin.sql @@ -416,3 +416,19 @@ VACUUM brintest; -- force a summarization cycle in brinidx UPDATE brintest SET int8col = int8col * int4col; UPDATE brintest SET textcol = '' WHERE textcol IS NOT NULL; + +-- Tests for brin_summarize_new_values +CREATE TABLE brin_example (id int); +CREATE INDEX btree_index ON brin_example(id); +CREATE INDEX brin_index ON brin_example USING brin(id); +-- Checks on relation type +SELECT brin_summarize_new_values('brin_example'::regclass); -- error +SELECT brin_summarize_new_values('btree_index'::regclass); -- error +SELECT brin_summarize_new_values('brin_index'::regclass); -- ok +-- Permission checks +CREATE ROLE brin_role; +SET SESSION AUTHORIZATION brin_role; +SELECT brin_summarize_new_values('brin_index'::regclass); -- error +RESET SESSION AUTHORIZATION; +DROP ROLE brin_role; +DROP TABLE brin_example; -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] 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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers