From: | Zdenek Kotala <Zdenek(dot)Kotala(at)Sun(dot)COM> |
---|---|
To: | Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com> |
Cc: | PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: New FSM patch |
Date: | 2008-09-16 20:40:46 |
Message-ID: | 48D019CE.4090108@sun.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | 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
From | Date | Subject | |
---|---|---|---|
Next Message | Tom Lane | 2008-09-16 21:52:45 | Re: Patch for SQL-standard negative valued year-month literals |
Previous Message | Ron Mayer | 2008-09-16 20:29:01 | Patch for SQL-standard negative valued year-month literals |