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
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
Attachments:
20151227_brin_new_values_fix.patchtext/x-patch; charset=US-ASCII; name=20151227_brin_new_values_fix.patchDownload
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;
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
Attachments:
document-summarize.patchtext/x-diff; charset=us-asciiDownload
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.
</para>
+
+ <sect2 id="brin-operation">
+ <title>Index Maintenance</title>
+
+ <para>
+ 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 <function>brin_summarize_new_pages(regclass)</function> function,
+ or automatically when <command>VACUUM</command> processes the table.
+ </para>
+ </sect2>
</sect1>
<sect1 id="brin-builtin-opclasses">
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;
</sect2>
+ <sect2>
+ <title>Indexing Helper Functions</title>
+
+ <indexterm>
+ <primary>brin_summarize_new_values</primary>
+ </indexterm>
+
+ <para>
+ <xref linkend="functions-admin-indexing"> shows the functions
+ available for index maintenance tasks.
+ </para>
+
+ <table id="functions-admin-indexing">
+ <title>Indexing Helper Functions</title>
+ <tgroup cols="3">
+ <thead>
+ <row><entry>Name</entry> <entry>Return Type</entry> <entry>Description</entry></row>
+ </thead>
+
+ <tbody>
+ <row>
+ <entry>
+ <literal><function>brin_summarize_new_values(<parameter>index_oid</parameter>)</function></literal>
+ </entry>
+ <entry><type>integer</type></entry>
+ <entry>summarize page ranges not already summarized</entry>
+ </row>
+ </tbody>
+ </tgroup>
+ </table>
+
+ <para>
+ <function>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.
+ </para>
+
+ </sect2>
+
</sect1>
<sect1 id="functions-trigger">
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers