Re: New FSM patch - Mailing list pgsql-hackers

Hi Heikki,

I'm still work on performance test, but I have following 
comments/questions/suggestion:

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

2) I suggest to renema following functions:

GetParentNo -> FSMGetParentPageNo
GetChildNo ->  FSMGetChildPageNo
GetFSMBlockNumber -> FSMGetBlockNumber


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.

4) I suggest to create structure

struct foo {    int level;int logpageno;int slot;
}

5) I see potential infinite recursive loop in fsm_search.

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


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.



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



pgsql/src/backend/storage/buffer/bufmgr.c
<http://reviewdemo.postgresql.org/r/27/#comment40>
    Extra space



pgsql/src/backend/storage/buffer/bufmgr.c
<http://reviewdemo.postgresql.org/r/27/#comment41>
    Extra space



pgsql/src/backend/storage/buffer/bufmgr.c
<http://reviewdemo.postgresql.org/r/27/#comment42>
    Extra space



pgsql/src/backend/storage/buffer/bufmgr.c
<http://reviewdemo.postgresql.org/r/27/#comment43>
    Extra space



pgsql/src/backend/storage/buffer/bufmgr.c
<http://reviewdemo.postgresql.org/r/27/#comment44>
    Extra space



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



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



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



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



pgsql/src/include/storage/freespace.h
<http://reviewdemo.postgresql.org/r/27/#comment35>
    Cleanup



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.




-- 
Zdenek Kotala              Sun Microsystems
Prague, Czech Republic     http://sun.com/postgresql