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

Reply via email to