Refactoring xlogutils.c
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.
I'm satisfied enough with it that I plan to commit it in a few days,
barring objections.
Summary of changes:
- XLogOpenRelation is gone. XLogReadBuffer now takes a RelFileNode as
argument, instead of Relation. Fix all callers.
- For the routines that need a fake, or lightweight, Relation struct
anyway, there's a new function called
XLogFakeRelcacheEntry(RelFileNode), that returns a palloc'd Relation struct.
- Add ReadBufferWithoutRelcache variant of ReadBuffer, that takes
RelFileNode instead of Relation as argument. This is what XLogReadBuffer
uses internally.
The only user-visible changes are these error message changes:
1. "invalid page header in block %u of relation %s" and "unexpected data
beyond EOF in block %u of relation \"%s\" messages, emitted from
ReadBuffer, no longer print the relation name, but only its relfilenode.
(This is because relation name is no longer conveniently available where
that message is emitted. It could be passed there if necessary, but it
doesn't seem worth the extra complexity)
2. elog("failed to additem to index page in \"%s\""), in gistutil.c. For
similar reasons, no longer prints the relation name. Unfortunately,
gistfillbuffer() doesn't know the relfilenode either. Again we could
pass it, but it doesn't seem worth the extra complexity, given that this
is just a "should never happen" elog, and the user should usually know
which index is at fault from the context information.
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
Attachments:
xlogutils-refactor-2.patchtext/x-diff; name=xlogutils-refactor-2.patchDownload+616-636
"Heikki Linnakangas" <heikki@enterprisedb.com> 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?
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.
regards, tom lane
- For the routines that need a fake, or lightweight, Relation struct
anyway, there's a new function called
XLogFakeRelcacheEntry(RelFileNode), that returns a palloc'd Relation
struct.
Is that fake structure applicable for ReadBuffer()?
ginContinueSplit() calls findParents() with a fake Relation struct, and
findParents will call ReadBuffer()...
GiST's function gistContinueInsert() is working by similar way.
--
Teodor Sigaev E-mail: teodor@sigaev.ru
WWW: http://www.sigaev.ru/
Teodor Sigaev wrote:
- For the routines that need a fake, or lightweight, Relation struct
anyway, there's a new function called
XLogFakeRelcacheEntry(RelFileNode), that returns a palloc'd Relation
struct.Is that fake structure applicable for ReadBuffer()?
Yes.
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
Tom Lane wrote:
"Heikki Linnakangas" <heikki@enterprisedb.com> 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
"Heikki Linnakangas" <heikki@enterprisedb.com> writes:
Tom Lane wrote:
The code motion in md.c looks fairly bogus; was that a copy-and-paste
error?
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.
Oh, I see --- I misread the patch the first time. That seems like a
reasonable change.
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.
I don't believe there is any way to call mdcreate except through
smgrcreate, so this is probably not a problem.
regards, tom lane