Re: [HACKERS] 9.5rc1 brin_summarize_new_values

[Tom Lane]

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

[Alvaro Herrera]

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

[Alvaro Herrera]

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

[Michael Paquier]

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

Re: [HACKERS] 9.5rc1 brin_summarize_new_values

[Tom Lane]

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

[Alvaro Herrera]

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

[Jeff Janes]

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]

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

[HACKERS] 9.5rc1 brin_summarize_new_values

[Jeff Janes]

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