Re: New FSM patch - Mailing list 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