Re: Refactoring xlogutils.c

Tom Lane wrote:
> "Heikki Linnakangas"  writes:
>> Attached is an updated version of my patch to refactor the
>> XLogOpenRelation/XLogReadBuffer interface, in preparation for the 
>> relation forks patch, and subsequently the FSM rewrite patch.
> 
> The code motion in md.c looks fairly bogus; was that a copy-and-paste
> error?

This one?

> *** a/src/backend/storage/smgr/md.c
> --- b/src/backend/storage/smgr/md.c
> ***************
> *** 208,216 **** mdcreate(SMgrRelation reln, bool isRedo)
>         char       *path;
>         File            fd;
>   
> -       if (isRedo && reln->md_fd != NULL)
> -               return;                                 /* created and opened already... */
> - 
>         Assert(reln->md_fd == NULL);
>   
>         path = relpath(reln->smgr_rnode);
> --- 208,213 ----

That's intentional. That check has been moved to smgrcreate(), so that 
it's done before calling TablespaceCreateDbSpace(). The reason for that 
is that smgrcreate() is now called for every XLogReadBuffer() 
invocation, so we want to make it exit quickly if there's nothing to do.

On second look though, I think we should actually leave that check in 
mdcreate(). Even though it'd be dead code since we do the same check in 
smgrcreate already, removing it changes the semantics of mdcreate(): 
it'd no longer be acceptable to call mdcreate() if the file is open already.

> Otherwise it looks pretty sane, but I have one stylistic gripe:
> I'm dubious about your willingness to assume that pfree() is enough for
> getting rid of a fake relcache entry.  Relcache entries are complex
> beasts and it may not continue to work to do that.  It's also possible
> that we'd have additional resources attached to them someday.  So
> I think it'd be worth having a FreeFakeRelcacheEntry() routine to
> localize the knowledge of how to get rid of them.

Yeah, I guess you're right.

-- 
   Heikki Linnakangas
   EnterpriseDB   http://www.enterprisedb.com