Re: [PATCHES] Split _bt_insertonpg to two functions

2007-03-03 Thread Bruce Momjian

Patch applied.  Thanks.

---


Heikki Linnakangas wrote:
> Here's a patch that:
> 
> Moves the logic to find a page with enough room from _bt_insertonpg to a 
> new function, _bt_findinsertloc. It makes the code more readable, and 
> simplifies the forthcoming Grouped Index Tuples patch.
> 
> Also, the insert location within page used to be calculated twice for 
> unique indexes, once in _bt_checkunique and second time in 
> _bt_insertonpg. That's a waste of cycles, and this patch fixes that.
> 
> 
> I couldn't measure a difference with pgbench, but this micro-benchmark 
> shows it:
> 
>  > psql postgres -c "CREATE TABLE inserttest (i int PRIMARY KEY);"
>  > psql postgres -c "TRUNCATE inserttest; checkpoint;"; sync
>  > time ~/pgsql.cvshead/bin/psql postgres -c "TRUNCATE inserttest; 
> INSERT INTO inserttest SELECT a FROM generate_series(1,100) a;"
> 
> Without patch:real0m7.260s
> With patch:   real0m6.963s
> 
> On my laptop, fsync=off, full_page_writes=off, checkpoint_segments = 10, 
> to remove any other variables.
> 
> It's not a huge difference, but it's worth having, and performance 
> wasn't the main motivation of the patch anyway.
> 
> -- 
>Heikki Linnakangas
>EnterpriseDB   http://www.enterprisedb.com

[ text/x-patch is unsupported, treating like TEXT/PLAIN ]

> Index: src/backend/access/nbtree/nbtinsert.c
> ===
> RCS file: 
> /home/hlinnaka/pgcvsrepository/pgsql/src/backend/access/nbtree/nbtinsert.c,v
> retrieving revision 1.152
> diff -c -r1.152 nbtinsert.c
> *** src/backend/access/nbtree/nbtinsert.c 21 Feb 2007 20:02:17 -  
> 1.152
> --- src/backend/access/nbtree/nbtinsert.c 26 Feb 2007 09:37:16 -
> ***
> *** 46,58 
>   static Buffer _bt_newroot(Relation rel, Buffer lbuf, Buffer rbuf);
>   
>   static TransactionId _bt_check_unique(Relation rel, IndexTuple itup,
> !  Relation heapRel, Buffer buf,
>ScanKey itup_scankey);
>   static void _bt_insertonpg(Relation rel, Buffer buf,
>  BTStack stack,
> -int keysz, ScanKey scankey,
>  IndexTuple itup,
> !OffsetNumber afteritem,
>  bool split_only_page);
>   static Buffer _bt_split(Relation rel, Buffer buf, OffsetNumber firstright,
> OffsetNumber newitemoff, Size newitemsz,
> --- 46,63 
>   static Buffer _bt_newroot(Relation rel, Buffer lbuf, Buffer rbuf);
>   
>   static TransactionId _bt_check_unique(Relation rel, IndexTuple itup,
> !  Relation heapRel, Buffer buf, OffsetNumber 
> ioffset,
>ScanKey itup_scankey);
> + static void _bt_findinsertloc(Relation rel,
> +   Buffer *bufptr, 
> +   OffsetNumber *offsetptr,
> +   int keysz,
> +   ScanKey scankey,
> +   IndexTuple newtup);
>   static void _bt_insertonpg(Relation rel, Buffer buf,
>  BTStack stack,
>  IndexTuple itup,
> !OffsetNumber newitemoff,
>  bool split_only_page);
>   static Buffer _bt_split(Relation rel, Buffer buf, OffsetNumber firstright,
> OffsetNumber newitemoff, Size newitemsz,
> ***
> *** 86,91 
> --- 91,97 
>   ScanKey itup_scankey;
>   BTStack stack;
>   Buffer  buf;
> + OffsetNumber offset;
>   
>   /* we need an insertion scan key to do our search, so build one */
>   itup_scankey = _bt_mkscankey(rel, itup);
> ***
> *** 94,99 
> --- 100,107 
>   /* find the first page containing this key */
>   stack = _bt_search(rel, natts, itup_scankey, false, &buf, BT_WRITE);
>   
> + offset = InvalidOffsetNumber;
> + 
>   /* trade in our read lock for a write lock */
>   LockBuffer(buf, BUFFER_LOCK_UNLOCK);
>   LockBuffer(buf, BT_WRITE);
> ***
> *** 128,134 
>   {
>   TransactionId xwait;
>   
> ! xwait = _bt_check_unique(rel, itup, heapRel, buf, itup_scankey);
>   
>   if (TransactionIdIsValid(xwait))
>   {
> --- 136,143 
>   {
>   TransactionId xwait;
>   
> ! offset = _bt_binsrch(rel, buf, natts, itup_scankey, false);
> ! xwait = _bt_check_unique(rel, itup, heapRel, buf, offset, 
> itup_scankey);
>   
>   if (TransactionIdIsValid(xwait))
>   {
> ***
> *** 142,148 
>   }
>   
>   /* do the insertion */
> ! _bt_insertonpg(rel, buf, stack, natts, itup_scankey, itup, 0, false);
>   
>   /* be tidy */
>   _bt_freestac

Re: [PATCHES] Split _bt_insertonpg to two functions

2007-02-28 Thread Bruce Momjian

Your patch has been added to the PostgreSQL unapplied patches list at:

http://momjian.postgresql.org/cgi-bin/pgpatches

It will be applied as soon as one of the PostgreSQL committers reviews
and approves it.

---


Heikki Linnakangas wrote:
> Here's a patch that:
> 
> Moves the logic to find a page with enough room from _bt_insertonpg to a 
> new function, _bt_findinsertloc. It makes the code more readable, and 
> simplifies the forthcoming Grouped Index Tuples patch.
> 
> Also, the insert location within page used to be calculated twice for 
> unique indexes, once in _bt_checkunique and second time in 
> _bt_insertonpg. That's a waste of cycles, and this patch fixes that.
> 
> 
> I couldn't measure a difference with pgbench, but this micro-benchmark 
> shows it:
> 
>  > psql postgres -c "CREATE TABLE inserttest (i int PRIMARY KEY);"
>  > psql postgres -c "TRUNCATE inserttest; checkpoint;"; sync
>  > time ~/pgsql.cvshead/bin/psql postgres -c "TRUNCATE inserttest; 
> INSERT INTO inserttest SELECT a FROM generate_series(1,100) a;"
> 
> Without patch:real0m7.260s
> With patch:   real0m6.963s
> 
> On my laptop, fsync=off, full_page_writes=off, checkpoint_segments = 10, 
> to remove any other variables.
> 
> It's not a huge difference, but it's worth having, and performance 
> wasn't the main motivation of the patch anyway.
> 
> -- 
>Heikki Linnakangas
>EnterpriseDB   http://www.enterprisedb.com


> 
> ---(end of broadcast)---
> TIP 7: You can help support the PostgreSQL project by donating at
> 
> http://www.postgresql.org/about/donate

-- 
  Bruce Momjian  <[EMAIL PROTECTED]>  http://momjian.us
  EnterpriseDB   http://www.enterprisedb.com

  + If your life is a hard drive, Christ can be your backup. +

---(end of broadcast)---
TIP 7: You can help support the PostgreSQL project by donating at

http://www.postgresql.org/about/donate