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

Re: FSM rewrite: doc changes

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com>
Cc: PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: FSM rewrite: doc changes
Date: 2008-09-28 17:15:49
Message-ID: 9258.1222622149@sss.pgh.pa.us (view raw or flat)
Thread:
Lists: pgsql-hackers
Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com> writes:
> To keep everyone who's interested up-to-date, attached is the latest 
> patch. ...
> I find it a bit disturbing that a documentation patch actually removes 
> more lines from the manual than adds, but it's quite understandable 
> because it's no longer necessary to explain the two GUC options that 
> used to be quite important :-). Comments welcome.

Well, this patch isn't actually supposed to have user-visible impact
other than eliminating a couple of troublesome configuration settings.
So it's entirely expected for the docs to get shorter ;-)

I did another pass of code-reading, and found a lot of nitpicks and
some not-so-trivial issues.  In no particular order:


Copyright in indexfsm.c is a year off.

InitIndexFreeSpaceMap should have a comment

The comment for RecordFreeIndexPage gives the function's name incorrectly.

InitFreeSpaceMap() should be explicitly declared as taking void in its
definition.

FreeSpaceMapTruncateRel seems to have a bug in its early-exit test: in the
case where the number of FSM blocks stays the same, it fails to zero out slots
in the last block.  I also think it's got an off-by-one problem in figuring
the number of FSM blocks: for the normal case where the new heap end is in
the middle of a FSM block, shouldn't new_nfsmblocks be one larger than it
is?  The case where nblocks is an exact multiple of SlotsPerFSMPage would
need to be special-cased to be exactly correct, though I see no real harm in
letting the FSM be left one page too big in that case.

The patch shouldn't be touching bufmgr.c at all any more --- or at least, none
of the diffs there are improvements.

Docs for contrib/pageinspect still need work: the 3-parameter form of
get_raw_page isn't documented, nor the fork behavior of the 2-parameter form.

In gistvacuum.c, you've removed the code that adjusts totFreePages to not
count pages truncated away.  I think you could just subtract the number of
truncated pages from it, since they must have been counted in it earlier.
(ginvacuum.c seems to get this right.)

I do not like the kluge in heap_xlog_clean one bit, and think it's unnecessary
anyway since we are not relying on the FSM to be accurate.  Suggest reverting
the heapam.c changes except for heap_sync().

rd_fsm_nblocks_cache should be reset in the places where rd_targblock is.
You seem to have tracked the clearings of rd_smgr which is not the right
thing at all.

I see you renamed "next", which is good, but the README isn't up to speed on
it and a lot of the comments aren't either.

Since fp_next_slot is signed, the sanity check in fsm_search_avail had better
include "target < 0".

The new search algorithm in fsm_search_avail still doesn't work.  Consider
what happens when the target is the rightmost slot on the page; it certainly
won't wrap properly.

fsm_truncate_avail seems quite broken: it's clearing the whole page always.

In fsm_rebuild_page, surely we needn't check "if (lchild < NodesPerPage)".
Also you probably ought to make it
		if (fsmpage->fp_nodes[nodeno] != newvalue)
		{
			fsmpage->fp_nodes[nodeno] = newvalue;
			changed = true;
		}
to avoid useless write traffic into a shared buffer.

I think DEPTH should be a macro not a static int; it's certainly
reducible to a compile-time constant.  Also I wonder whether you
really need the SlotsPerFSMPagePowers[] array at all (and if not,
you could get rid of InitFreeSpaceMap).  It's used in only one
place and it seems a bit hard to argue that a multiplication loop
really needs to be avoided there --- the division loop that comes
after it will cost a lot more, and in any case both are negligible
compared to the shared buffer fetch that's about to occur.

This test in fsm_space_needed_to_cat:
	if (needed >= (FSM_CATEGORIES - 1) * FSM_CAT_STEP)
		elog(ERROR, "invalid FSM request size");
reveals a rather fundamental problem: it is clearly possible
for this test to fail on valid request sizes, because the page
header overhead is less than FSM_CAT_STEP (especially if BLCKSZ
is more than 8K).  I'm not sure about a really clean solution
here.  We could offset the needed_to_cat and avail_to_cat
calculations so that category 255 corresponds exactly to the
maximum possible free space, but that requires assuming that FSM
knows exactly what that is, which is a bit unpleasant.  Thoughts?

It seems a bit schizophrenic that fsm_search_avail takes a Buffer
when all the other functions in fsmpage.c take Page arguments.
I see why fsm_search_avail needs to do that, but maybe it'd be
better if the other functions did too?

fsm_search() should not take addr as an argument, since it has a
built-in assumption that it is started at the root.

I find the use of eof as both a local variable and a parameter in
fsm_vacuum_page to be pretty poor programming practice.  Maybe call
the parameter eof_p?

Shouldn't fsm_redo include a FreeFakeRelcacheEntry call?

			regards, tom lane

In response to

Responses

pgsql-hackers by date

Next:From: Tom LaneDate: 2008-09-28 18:02:25
Subject: Re: [PATCHES] Infrastructure changes for recovery
Previous:From: Hannu KrosingDate: 2008-09-28 11:45:03
Subject: Re: Null row vs. row of nulls in plpgsql

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