Tom Lane wrote:
One thing I did *not* like was changing the FSM API to refer to Relation
rather than RelFileNode --- I don't believe that's a good idea at all.
In particular, consider what happens during TRUNCATE or CLUSTER: it's
not very clear how you'll tell the versions of the relation apart.
If you want to push the FSM API up to use SMgrRelation instead of
RelFileNode, that'd be okay, but not Relation. (Essentially the
distinction I'm trying to preserve here is logical vs physical
relation.)
Oh really? I'm quite fond of the new API. From a philosophical point of
view, in the new world order, the FSM is an integral part of a relation,
not something tacked on the physical layer. TRUNCATE and CLUSTER will
need to truncate and truncate+recreate the FSM file, respectively. The
FSM fork is on an equal footing with the main fork: when TRUNCATE swaps
the relation file, a new FSM fork is created as well, and there's no way
or need to access the old file anymore. When a relation is moved to
another tablespace, the FSM fork is moved as well, and while the
RelFileNode changes at that point, the logical Relation is the same.
Besides, Relation contains a bunch of very handy fields. pgstat_info in
particular, which is needed if we want to collect pgstat information
about FSM, and I think we will. I might also want add a field like
rd_amcache there, for the FSM: I'm thinking of implementing something
like the fastroot thing we have in b-tree, and we might need some other
per-relation information there as well.
The XLogOpenRelationWithFork stuff needs to be re-thought also,
as again this is blurring the question of what's a logical and
what's a physical relation --- and if forknum isn't part of the
relation ID, that API is wrong either way. I'm not sure about
a good solution in this area, but I wonder if the right answer
might be to make the XLOG replay stuff use SMgrRelations instead
of bogus Relations. IIRC the replay code design predates the
existence of SMgrRelation, so maybe we need a fresh look there.
Agreed, I'm not happy with that part either. I tried to do just what you
suggest, make XLOG replay stuff deal with SMgrRelations instead of the
lightweight relcache, and it did look good until I got to refactoring
btree_xlog_cleanup() (GIN/GiST has the same problem, IIRC).
btree_xlog_cleanup() uses the same functions as the normal-operation
code to insert pointers to parent pages, which operates on Relation.
That started to become really hairy to solve without completely
bastardizing the normal code paths.
Hmm. One idea would be to still provide a function to create a fake
RelationData struct from SMgrRelation, which the redo function can call
in that kind of situations.
(On closer look, XLogOpenRelationWithFork seems unused anyway
That's just because FSM WAL-logging hasn't been implemented yet.
One really trivial thing that grated on me was
+ /*
+ * In a few places we need to loop through 0..MAX_FORKS to discover which
+ * forks exists, so we should try to keep this number small.
+ */
+ #define MAX_FORKS (FSM_FORKNUM + 1)
I think you should either call it MAX_FORK (equal to the last fork
number) or NUM_FORKS (equal to last fork number plus one). As is,
it's just confusing.
Agreed, will fix.
And the comment is flat out wrong for the current usage.
What's described in the comment is done in ATExecSetTableSpace. I grant
you that there's many other usages for it. I'll work on the comment.
BTW, it would probably be a good idea to try to get the fork access
API committed before you work on FSM. Whenever you can break a
big patch into successive sections, it's a good idea, IMHO. I don't
think there's any doubt that we are going to go in this direction,
so I see no objection to committing fork-based API revisions in advance
of having any real use for them.
Yep. I'll develop them together for now, but will separate them when the
fork stuff is ripe for committing.
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches
