Обсуждение: Patch for pgstatindex to fix a bug reporting a value of strange leaf_fragmentation

Hello.

I found a bug in contrib/pgstatindex.c to reports a strange value of
leaf_fragmentation with some cases.
#Look the following test example.

In GetBTPageStatistics(),  stat->fragments is not initialized properly
so that invalid leaf_fragments values are inserted in results.
For example, GetBTPageStatistics() read a dead_leaf_block
(stat->type = 'd';) first, stat->fragments hold a strange value because
stat->fragments is not initialized before "return true"(pgstatindex.c
164th line)
And then, the strange value is just inserted in result. Under normal
circumstances, stat.fragments must be incremented only when stat->type
 is 'l'.
So pgstatindex reports strange leaf_fragmentation.

I made the patch.
Please confirm this.

Best regards.

======== Test (postgresql8.2.1)========
test=# CREATE TABLE test (id int);
CREATE TABLE

test=# INSERT INTO test select generate_series(1,1000);
INSERT 0 1000

test=# CREATE INDEX idx_test ON test(id);
CREATE INDEX

test=# SELECT leaf_fragmentation from pgstatindex('idx_test');
-[ RECORD 1 ]------+--
leaf_fragmentation | 0

test=# DELETE FROM test;
DELETE 1000

test=# SELECT leaf_fragmentation from pgstatindex('idx_test');
-[ RECORD 1 ]------+--
leaf_fragmentation | 0

test=# VACUUM test;
VACUUM

test=# SELECT leaf_fragmentation from pgstatindex('idx_test');
-[ RECORD 1 ]------+------------
leaf_fragmentation | 28943878400 <- Invalid value
======== Test (postgresql8.2.1)========

=========gdb============
(gdb) b GetBTPageStatistics
Breakpoint 4 at 0xfd8126: file pgstatindex.c, line 145.
(gdb) c
Continuing.

Breakpoint 4, GetBTPageStatistics (blkno=1, buffer=101, stat=0xbfc3fbe4)
   at pgstatindex.c:145
145             Page            page = BufferGetPage(buffer);
(gdb) print stat.fragments
$4 = 144719392  <- Already have a invalid value.

(snip)

Breakpoint 3, GetBTPageStatistics (blkno=1, buffer=<value optimized out>,
   stat=0xbfc3fbe4) at pgstatindex.c:161
161             if (P_ISDELETED(opaque))
(gdb) n
163                     stat->type = 'd';

(snip)

pgstatindex (fcinfo=0xbfc3fc70) at pgstatindex.c:297
297                     switch (stat.type)
(gdb) n
300                                     indexStat.deleted_pages++;
(gdb) n
319                     indexStat.fragments += stat.fragments;
(gdb) print stat.fragments
$3 = 144719392  <- Just inserts inavlid value in result even if stat.type is 'dead'.
=========gdb============

=========patch test============
test=# CREATE TABLE test2 (id int);
CREATE TABLE
test=# INSERT INTO test2 select generate_series(1,1000);
INSERT 0 1000
test=# CREATE INDEX idx_test2 ON test2(id);
CREATE INDEX
test=# DELETE FROM test2;
DELETE 1000
test=# VACUUM test2;
VACUUM
test=# SELECT leaf_fragmentation from pgstatindex('idx_test2');
-[ RECORD 1 ]------+--
leaf_fragmentation | 0 <- OK

test=# INSERT INTO test2 select generate_series(1,1000);
INSERT 0 1000
test=# SELECT leaf_fragmentation from pgstatindex('idx_test2');
-[ RECORD 1 ]------+--
leaf_fragmentation | 0 <- OK

test=# INSERT INTO test2 select generate_series(1,1000);
INSERT 0 1000
test=# SELECT leaf_fragmentation from pgstatindex('idx_test2');
-[ RECORD 1 ]------+------
leaf_fragmentation | 28.57 <- OK.
=========patch test============


--
NTT OSS Center
Tatsuhito Kasahara

kasahara.tatsuhito _at_ oss.ntt.co.jp
*** pgstatindex.c.org    2006-09-04 11:03:04.000000000 +0900
--- pgstatindex.c    2007-03-08 01:06:14.000000000 +0900
***************
*** 154,159 ****
--- 154,160 ----
      stat->max_avail = BLCKSZ - (BLCKSZ - phdr->pd_special + SizeOfPageHeaderData);

      stat->dead_items = stat->live_items = 0;
+     stat->fragments = 0;

      stat->page_size = PageGetPageSize(page);

***************
*** 187,193 ****
       * it means a fragmentation.
       *----------------------------------------------
       */
-     stat->fragments = 0;
      if (stat->type == 'l')
      {
          if (opaque->btpo_next != P_NONE && opaque->btpo_next < blkno)
--- 188,193 ----

Tatsuhito Kasahara wrote:
> Hello.
>
> I found a bug in contrib/pgstatindex.c to reports a strange value of
> leaf_fragmentation with some cases.
> #Look the following test example.
>
> In GetBTPageStatistics(),  stat->fragments is not initialized properly
> so that invalid leaf_fragments values are inserted in results.

Right.  Checking that code, it seems btpo.xact is also being incorrectly
handled, is it not?  Incidentally, the return value seems a bit useless.

--
Alvaro Herrera                                http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Alvaro Herrera wrote:

> Right.  Checking that code, it seems btpo.xact is also being incorrectly
> handled, is it not?  Incidentally, the return value seems a bit useless.

Then again, this looks bogus as well.  (Sorry for the unidiff -- I had to
use "interdiff" to get it).

--
Alvaro Herrera                                http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Hi.

Alvaro Herrera wrote:
>> In GetBTPageStatistics(),  stat->fragments is not initialized properly
>> so that invalid leaf_fragments values are inserted in results.
>
> Right.  Checking that code, it seems btpo.xact is also being incorrectly
> handled, is it not?
Yeah, I think so.

> Incidentally, the return value seems a bit useless.
Exactly. "Void" is enough.

--
NTT OSS Center
Tatsuhito Kasahara

kasahara.tatsuhito _at_ oss.ntt.co.jp

Tatsuhito Kasahara wrote:
> Hello.
>
> I found a bug in contrib/pgstatindex.c to reports a strange value of
> leaf_fragmentation with some cases.

Applied to HEAD and 8.2, thanks.

--
Alvaro Herrera                                http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.