Skip site navigation (1) Skip section navigation (2)

Re: New FSM patch

From: Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com>
To: Zdenek Kotala <Zdenek(dot)Kotala(at)Sun(dot)COM>
Cc: PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: New FSM patch
Date: 2008-09-17 08:06:50
Message-ID: 48D0BA9A.1070000@enterprisedb.com (view raw or flat)
Thread:
Lists: pgsql-hackers
Zdenek Kotala wrote:
> I'm still work on performance test, but I have following 
> comments/questions/suggestion:

Thanks!

> 1)
> #define NodesPerPage (BLCKSZ - SizeOfPageHeaderData - 
> offsetof(FSMPageData, fp_nodes))
> 
> should be
> 
> #define NodesPerPage (BLCKSZ - MAXALIGN(SizeOfPageHeaderData) - 
> offsetof(FSMPageData, fp_nodes))
> 
> See how PageGetContents is defined

Yes, good catch.

> 2) I suggest to renema following functions:
> 
> GetParentNo -> FSMGetParentPageNo
> GetChildNo ->  FSMGetChildPageNo
> GetFSMBlockNumber -> FSMGetBlockNumber

Well, they're just static functions. But sure, why not.

> 3) I'm not happy much with idea that page contains data and they are 
> "invisible". special, lower or upper is unset. It seems like empty page. 
> I know that it is used in hash index implementation as well, but it 
> could be fixed too.
> 
> I suggest to set special and upper correctly (or only upper). lower 
> should indicate that there are not linp.

Hmm. In B-tree metapage, pd_lower is set to point to the end of the 
struct that's stored there. It's because that allows the xlog code to 
skip the unused space there in full page images, but that's not 
applicable for FSM pages.

I think I'll fix that so that the data is stored in the special part, 
and the special part fills the whole page.

> 4) I suggest to create structure
> 
> struct foo {
>     int level;
>     int logpageno;
>     int slot;
> }

Yes, that might be helpful.

> 5) I see potential infinite recursive loop in fsm_search.

Yes, that's quite subtle. The recursion should end eventually, because 
whenever we reach a dead-end when we descend the tree, we fix the upper 
nodes so that we shouldn't take that dead-end path on the next iteration.

That said, perhaps it would be a good idea to put a counter there and 
stop after say a few thousand iterations, just in case.. In any case, 
looks like it needs more comments.

I think I'll restructure that into a loop, instead of recursion.

> 6) Does FANOUT^4 fit into int? (by the way what FANOUT means?)

FANOUT is just an alias for LeafNodesPerPage. It's the number of child 
pages a non-leaf FSM page has (or can have).

No, FANOUT^4 doesn't fit in int, good catch. Actually, FANOUTPOWERS 
table doesn't need to go that far, so that's just a leftover. It only 
needs to have DEPTH elements. However, we have the same problem if 
DEPTH==3, FANOUT^4 will not fit into int. I put a comment there. 
Ideally, the 4th element would be #iffed out, but I couldn't immediately 
figure out how to do that.

> And there are more comments on rcodereview:
> 
> pgsql/src/backend/catalog/index.c
> <http://reviewdemo.postgresql.org/r/27/#comment45>
> 
>     Strange comment? Looks like copy paste error.

That function, setNewRelfilenode() is used for heaps as well, even 
though it's in index.c. I'll phrase the comment better..

> pgsql/src/backend/catalog/index.c
> <http://reviewdemo.postgresql.org/r/27/#comment47>
> 
>     ?RELKIND_INDEX?

No, that's correct, see above. The FSM is only created for heaps there, 
indexes are responsible for creating their own FSM if they need one. 
Hash indexes for example don't need one.

> pgsql/src/backend/storage/freespace/freespace.c
> <http://reviewdemo.postgresql.org/r/27/#comment37>
> 
>     Use shift, however compileer could optimize it anyway.

I think I'll leave it to the compiler, for the sake of readibility.

> pgsql/src/backend/storage/freespace/freespace.c
> <http://reviewdemo.postgresql.org/r/27/#comment38>
> 
>     Why?  ;-)

:-) Comment updated to:
  /* Can't ask for more space than the highest category represents */

> pgsql/src/backend/storage/freespace/freespace.c
> <http://reviewdemo.postgresql.org/r/27/#comment39>
> 
>     What's happen if FSM_FORKNUM does not exist?

smgrnblocks() will throw an error, I believe. Users of the FSM are 
responsible to create the FSM fork if they need it.

> pgsql/src/include/storage/bufmgr.h
> <http://reviewdemo.postgresql.org/r/27/#comment36>
> 
>     Need consolidate - forknum vs blockNum, zeroPage

What do you mean?

> pgsql/src/include/storage/lwlock.h
> <http://reviewdemo.postgresql.org/r/27/#comment49>
> 
>     Maybe better to use RESERVED to preserve lock numbers. It helps to 
> DTrace script be more backward compatible.

Hmm, each lightweight lock uses a few bytes of shared memory, but I 
guess that's insignificant. I'll do that and add a comment explaining 
why that's done.


Here's a new patch, updated per your comments.

PS. ReviewBoard seems to be quite nice for pointing out small changes 
like that.

-- 
   Heikki Linnakangas
   EnterpriseDB   http://www.enterprisedb.com

Attachment: fsm-lazy-4.patch.gz
Description: application/x-gzip (43.5 KB)

In response to

Responses

pgsql-hackers by date

Next:From: Simon RiggsDate: 2008-09-17 08:12:10
Subject: Re: Autovacuum and Autoanalyze
Previous:From: Heikki LinnakangasDate: 2008-09-17 07:09:13
Subject: Re: Autovacuum and Autoanalyze

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group