On Sat, Apr 24, 2010 at 11:02 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:
> Robert Haas <robertmh...@gmail.com> writes:
>> Pushing it into the RelFileNode has some advantages in terms of being
>> able to get at the information from everywhere, but one thing that
>> makes me think that's probably not a good decision is that we somtimes
>> WAL-log relfilenodes. And WAL-logging the value of the isTemp flag is
>> a waste, because if we're WAL-logging, it's zero.
>
> Yeah. I think we also use RelFileNode as a hash tag in places, and
> so adding a bool to it would be problematic for a couple of reasons:
> possibly uninitialized pad bytes, and uselessly incorporating more bytes
> into the hash calculation.
Right. I was thinking about the padding issue, too. I took a crack
at adding an isTemp argument to smgropen() and removing it everywhere
else, but it turns out this isn't as straightforward as it ought to be
because nbtsort.c, rewriteheap.c, and tablecmds.c feel entitled to
violate the abstraction layer by passing isTemp = true to smgrwrite()
and/or smgrextend() even when the real value is false, under the
assumption that the only thing isTemp is doing in those functions is
controlling fsync behavior. I think we'll have to replace the isTemp
argument to those functions with a boolean whose explicit charter is
to do what they're using it for.
Removing the isTemp from smgrtruncate() and smgrdounlink() argument
looks easy, though. WIP patch that takes it that far is attached.
[davidfetter: patch description for PWN is "WIP patch to push isTemp
down into the smgr layer"]
...Robert
diff --git a/src/backend/access/heap/visibilitymap.c b/src/backend/access/heap/visibilitymap.c
index d1f7bcc..9645c95 100644
--- a/src/backend/access/heap/visibilitymap.c
+++ b/src/backend/access/heap/visibilitymap.c
@@ -373,8 +373,7 @@ visibilitymap_truncate(Relation rel, BlockNumber nheapblocks)
}
/* Truncate the unused VM pages, and send smgr inval message */
- smgrtruncate(rel->rd_smgr, VISIBILITYMAP_FORKNUM, newnblocks,
- rel->rd_istemp);
+ smgrtruncate(rel->rd_smgr, VISIBILITYMAP_FORKNUM, newnblocks);
/*
* We might as well update the local smgr_vm_nblocks setting. smgrtruncate
diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
index 3cc9bdd..b318700 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -1317,13 +1317,13 @@ FinishPreparedTransaction(const char *gid, bool isCommit)
}
for (i = 0; i < ndelrels; i++)
{
- SMgrRelation srel = smgropen(delrels[i]);
+ SMgrRelation srel = smgropen(delrels[i], false);
ForkNumber fork;
for (fork = 0; fork <= MAX_FORKNUM; fork++)
{
if (smgrexists(srel, fork))
- smgrdounlink(srel, fork, false, false);
+ smgrdounlink(srel, fork, false);
}
smgrclose(srel);
}
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index b88cff2..2c5800d 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -4434,7 +4434,8 @@ xact_redo_commit(xl_xact_commit *xlrec, TransactionId xid, XLogRecPtr lsn)
/* Make sure files supposed to be dropped are dropped */
for (i = 0; i < xlrec->nrels; i++)
{
- SMgrRelation srel = smgropen(xlrec->xnodes[i]);
+ /* We know this can't be a temp rel, since it is xlog'd. */
+ SMgrRelation srel = smgropen(xlrec->xnodes[i], false);
ForkNumber fork;
for (fork = 0; fork <= MAX_FORKNUM; fork++)
@@ -4442,7 +4443,7 @@ xact_redo_commit(xl_xact_commit *xlrec, TransactionId xid, XLogRecPtr lsn)
if (smgrexists(srel, fork))
{
XLogDropRelation(xlrec->xnodes[i], fork);
- smgrdounlink(srel, fork, false, true);
+ smgrdounlink(srel, fork, true);
}
}
smgrclose(srel);
@@ -4537,7 +4538,8 @@ xact_redo_abort(xl_xact_abort *xlrec, TransactionId xid)
/* Make sure files supposed to be dropped are dropped */
for (i = 0; i < xlrec->nrels; i++)
{
- SMgrRelation srel = smgropen(xlrec->xnodes[i]);
+ /* We know this can't be a temp rel, since it is xlog'd. */
+ SMgrRelation srel = smgropen(xlrec->xnodes[i], false);
ForkNumber fork;
for (fork = 0; fork <= MAX_FORKNUM; fork++)
@@ -4545,7 +4547,7 @@ xact_redo_abort(xl_xact_abort *xlrec, TransactionId xid)
if (smgrexists(srel, fork))
{
XLogDropRelation(xlrec->xnodes[i], fork);
- smgrdounlink(srel, fork, false, true);
+ smgrdounlink(srel, fork, true);
}
}
smgrclose(srel);
diff --git a/src/backend/access/transam/xlogutils.c b/src/backend/access/transam/xlogutils.c
index 9ee2036..4462cfd 100644
--- a/src/backend/access/transam/xlogutils.c
+++ b/src/backend/access/transam/xlogutils.c
@@ -277,8 +277,8 @@ XLogReadBufferExtended(RelFileNode rnode, ForkNumber forknum,
Assert(blkno != P_NEW);
- /* Open the relation at smgr level */
- smgr = smgropen(rnode);
+ /* Open the relation at smgr level. It's xlog'd, thus not temp. */
+ smgr = smgropen(rnode, false);
/*
* Create the target file if it doesn't already exist. This lets us cope
diff --git a/src/backend/catalog/storage.c b/src/backend/catalog/storage.c
index ad376a1..a9602e6 100644
--- a/src/backend/catalog/storage.c
+++ b/src/backend/catalog/storage.c
@@ -103,7 +103,7 @@ RelationCreateStorage(RelFileNode rnode, bool istemp)
xl_smgr_create xlrec;
SMgrRelation srel;
- srel = smgropen(rnode);
+ srel = smgropen(rnode, istemp);
smgrcreate(srel, MAIN_FORKNUM, false);
if (!istemp)
@@ -283,7 +283,7 @@ RelationTruncate(Relation rel, BlockNumber nblocks)
}
/* Do the real work */
- smgrtruncate(rel->rd_smgr, MAIN_FORKNUM, nblocks, rel->rd_istemp);
+ smgrtruncate(rel->rd_smgr, MAIN_FORKNUM, nblocks);
}
/*
@@ -322,14 +322,11 @@ smgrDoPendingDeletes(bool isCommit)
SMgrRelation srel;
int i;
- srel = smgropen(pending->relnode);
+ srel = smgropen(pending->relnode, pending->isTemp);
for (i = 0; i <= MAX_FORKNUM; i++)
{
if (smgrexists(srel, i))
- smgrdounlink(srel,
- i,
- pending->isTemp,
- false);
+ smgrdounlink(srel, i, false);
}
smgrclose(srel);
}
@@ -456,7 +453,8 @@ smgr_redo(XLogRecPtr lsn, XLogRecord *record)
xl_smgr_create *xlrec = (xl_smgr_create *) XLogRecGetData(record);
SMgrRelation reln;
- reln = smgropen(xlrec->rnode);
+ /* We know this can't be a temp rel, since it is xlog'd. */
+ reln = smgropen(xlrec->rnode, false);
smgrcreate(reln, MAIN_FORKNUM, true);
}
else if (info == XLOG_SMGR_TRUNCATE)
@@ -465,7 +463,8 @@ smgr_redo(XLogRecPtr lsn, XLogRecord *record)
SMgrRelation reln;
Relation rel;
- reln = smgropen(xlrec->rnode);
+ /* We know this can't be a temp rel, since it is xlog'd. */
+ reln = smgropen(xlrec->rnode, false);
/*
* Forcibly create relation if it doesn't exist (which suggests that
@@ -475,7 +474,7 @@ smgr_redo(XLogRecPtr lsn, XLogRecord *record)
*/
smgrcreate(reln, MAIN_FORKNUM, true);
- smgrtruncate(reln, MAIN_FORKNUM, xlrec->blkno, false);
+ smgrtruncate(reln, MAIN_FORKNUM, xlrec->blkno);
/* Also tell xlogutils.c about it */
XLogTruncateRelation(xlrec->rnode, MAIN_FORKNUM, xlrec->blkno);
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 25b2807..aea7cc5 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -6981,7 +6981,7 @@ ATExecSetTableSpace(Oid tableOid, Oid newTableSpace)
newrnode = rel->rd_node;
newrnode.relNode = newrelfilenode;
newrnode.spcNode = newTableSpace;
- dstrel = smgropen(newrnode);
+ dstrel = smgropen(newrnode, rel->rd_istemp);
RelationOpenSmgr(rel);
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index caae936..f69e959 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -260,7 +260,7 @@ ReadBufferWithoutRelcache(RelFileNode rnode, bool isTemp,
{
bool hit;
- SMgrRelation smgr = smgropen(rnode);
+ SMgrRelation smgr = smgropen(rnode, isTemp);
return ReadBuffer_common(smgr, isTemp, forkNum, blockNum, mode, strategy,
&hit);
@@ -1838,7 +1838,7 @@ FlushBuffer(volatile BufferDesc *buf, SMgrRelation reln)
/* Find smgr relation for buffer */
if (reln == NULL)
- reln = smgropen(buf->tag.rnode);
+ reln = smgropen(buf->tag.rnode, false);
TRACE_POSTGRESQL_BUFFER_FLUSH_START(buf->tag.forkNum,
buf->tag.blockNum,
diff --git a/src/backend/storage/buffer/localbuf.c b/src/backend/storage/buffer/localbuf.c
index c5b6a2c..b036512 100644
--- a/src/backend/storage/buffer/localbuf.c
+++ b/src/backend/storage/buffer/localbuf.c
@@ -198,7 +198,7 @@ LocalBufferAlloc(SMgrRelation smgr, ForkNumber forkNum, BlockNumber blockNum,
SMgrRelation oreln;
/* Find smgr relation for buffer */
- oreln = smgropen(bufHdr->tag.rnode);
+ oreln = smgropen(bufHdr->tag.rnode, true);
/* And write... */
smgrwrite(oreln,
diff --git a/src/backend/storage/freespace/freespace.c b/src/backend/storage/freespace/freespace.c
index 579572f..b43a2ce 100644
--- a/src/backend/storage/freespace/freespace.c
+++ b/src/backend/storage/freespace/freespace.c
@@ -303,7 +303,7 @@ FreeSpaceMapTruncateRel(Relation rel, BlockNumber nblocks)
}
/* Truncate the unused FSM pages, and send smgr inval message */
- smgrtruncate(rel->rd_smgr, FSM_FORKNUM, new_nfsmblocks, rel->rd_istemp);
+ smgrtruncate(rel->rd_smgr, FSM_FORKNUM, new_nfsmblocks);
/*
* We might as well update the local smgr_fsm_nblocks setting.
diff --git a/src/backend/storage/smgr/md.c b/src/backend/storage/smgr/md.c
index 4163ca0..63d049b 100644
--- a/src/backend/storage/smgr/md.c
+++ b/src/backend/storage/smgr/md.c
@@ -794,8 +794,7 @@ mdnblocks(SMgrRelation reln, ForkNumber forknum)
* mdtruncate() -- Truncate relation to specified number of blocks.
*/
void
-mdtruncate(SMgrRelation reln, ForkNumber forknum, BlockNumber nblocks,
- bool isTemp)
+mdtruncate(SMgrRelation reln, ForkNumber forknum, BlockNumber nblocks)
{
MdfdVec *v;
BlockNumber curnblk;
@@ -839,7 +838,7 @@ mdtruncate(SMgrRelation reln, ForkNumber forknum, BlockNumber nblocks,
errmsg("could not truncate file \"%s\": %m",
FilePathName(v->mdfd_vfd))));
- if (!isTemp)
+ if (!reln->isTemp)
register_dirty_segment(reln, forknum, v);
v = v->mdfd_chain;
Assert(ov != reln->md_fd[forknum]); /* we never drop the 1st
@@ -864,7 +863,7 @@ mdtruncate(SMgrRelation reln, ForkNumber forknum, BlockNumber nblocks,
errmsg("could not truncate file \"%s\" to %u blocks: %m",
FilePathName(v->mdfd_vfd),
nblocks)));
- if (!isTemp)
+ if (!reln->isTemp)
register_dirty_segment(reln, forknum, v);
v = v->mdfd_chain;
ov->mdfd_chain = NULL;
@@ -1051,8 +1050,12 @@ mdsync(void)
* we couldn't safely do that.) Furthermore, in many cases
* the relation will have been dirtied through this same smgr
* relation, and so we can save a file open/close cycle.
+ *
+ * We will never be asked to fsync a temporary relation, so
+ * it's safe to pass false as the second argument to
+ * smgropen().
*/
- reln = smgropen(entry->tag.rnode);
+ reln = smgropen(entry->tag.rnode, false);
/*
* It is possible that the relation has been dropped or
diff --git a/src/backend/storage/smgr/smgr.c b/src/backend/storage/smgr/smgr.c
index 3b12cb3..cc72e5d 100644
--- a/src/backend/storage/smgr/smgr.c
+++ b/src/backend/storage/smgr/smgr.c
@@ -57,7 +57,7 @@ typedef struct f_smgr
BlockNumber blocknum, char *buffer, bool isTemp);
BlockNumber (*smgr_nblocks) (SMgrRelation reln, ForkNumber forknum);
void (*smgr_truncate) (SMgrRelation reln, ForkNumber forknum,
- BlockNumber nblocks, bool isTemp);
+ BlockNumber nblocks);
void (*smgr_immedsync) (SMgrRelation reln, ForkNumber forknum);
void (*smgr_pre_ckpt) (void); /* may be NULL */
void (*smgr_sync) (void); /* may be NULL */
@@ -131,7 +131,7 @@ smgrshutdown(int code, Datum arg)
* This does not attempt to actually open the object.
*/
SMgrRelation
-smgropen(RelFileNode rnode)
+smgropen(RelFileNode rnode, bool isTemp)
{
SMgrRelation reln;
bool found;
@@ -160,6 +160,7 @@ smgropen(RelFileNode rnode)
int forknum;
/* hash_search already filled in the lookup key */
+ reln->isTemp = isTemp;
reln->smgr_owner = NULL;
reln->smgr_targblock = InvalidBlockNumber;
reln->smgr_fsm_nblocks = InvalidBlockNumber;
@@ -323,7 +324,7 @@ smgrcreate(SMgrRelation reln, ForkNumber forknum, bool isRedo)
* already.
*/
void
-smgrdounlink(SMgrRelation reln, ForkNumber forknum, bool isTemp, bool isRedo)
+smgrdounlink(SMgrRelation reln, ForkNumber forknum, bool isRedo)
{
RelFileNode rnode = reln->smgr_rnode;
int which = reln->smgr_which;
@@ -331,7 +332,7 @@ smgrdounlink(SMgrRelation reln, ForkNumber forknum, bool isTemp, bool isRedo)
/* Close the fork */
(*(smgrsw[which].smgr_close)) (reln, forknum);
- smgr_internal_unlink(rnode, forknum, which, isTemp, isRedo);
+ smgr_internal_unlink(rnode, forknum, which, reln->isTemp, isRedo);
}
/*
@@ -455,14 +456,13 @@ smgrnblocks(SMgrRelation reln, ForkNumber forknum)
* The truncation is done immediately, so this can't be rolled back.
*/
void
-smgrtruncate(SMgrRelation reln, ForkNumber forknum, BlockNumber nblocks,
- bool isTemp)
+smgrtruncate(SMgrRelation reln, ForkNumber forknum, BlockNumber nblocks)
{
/*
* Get rid of any buffers for the about-to-be-deleted blocks. bufmgr will
* just drop them without bothering to write the contents.
*/
- DropRelFileNodeBuffers(reln->smgr_rnode, forknum, isTemp, nblocks);
+ DropRelFileNodeBuffers(reln->smgr_rnode, forknum, reln->isTemp, nblocks);
/*
* Send a shared-inval message to force other backends to close any smgr
@@ -479,8 +479,7 @@ smgrtruncate(SMgrRelation reln, ForkNumber forknum, BlockNumber nblocks,
/*
* Do the truncation.
*/
- (*(smgrsw[reln->smgr_which].smgr_truncate)) (reln, forknum, nblocks,
- isTemp);
+ (*(smgrsw[reln->smgr_which].smgr_truncate)) (reln, forknum, nblocks);
}
/*
diff --git a/src/include/storage/smgr.h b/src/include/storage/smgr.h
index cf248b8..6794c13 100644
--- a/src/include/storage/smgr.h
+++ b/src/include/storage/smgr.h
@@ -43,6 +43,9 @@ typedef struct SMgrRelationData
/* pointer to owning pointer, or NULL if none */
struct SMgrRelationData **smgr_owner;
+ /* is this a temporary relation? */
+ bool isTemp;
+
/*
* These next three fields are not actually used or manipulated by smgr,
* except that they are reset to InvalidBlockNumber upon a cache flush
@@ -70,7 +73,7 @@ typedef SMgrRelationData *SMgrRelation;
extern void smgrinit(void);
-extern SMgrRelation smgropen(RelFileNode rnode);
+extern SMgrRelation smgropen(RelFileNode rnode, bool isTemp);
extern bool smgrexists(SMgrRelation reln, ForkNumber forknum);
extern void smgrsetowner(SMgrRelation *owner, SMgrRelation reln);
extern void smgrclose(SMgrRelation reln);
@@ -78,7 +81,7 @@ extern void smgrcloseall(void);
extern void smgrclosenode(RelFileNode rnode);
extern void smgrcreate(SMgrRelation reln, ForkNumber forknum, bool isRedo);
extern void smgrdounlink(SMgrRelation reln, ForkNumber forknum,
- bool isTemp, bool isRedo);
+ bool isRedo);
extern void smgrextend(SMgrRelation reln, ForkNumber forknum,
BlockNumber blocknum, char *buffer, bool isTemp);
extern void smgrprefetch(SMgrRelation reln, ForkNumber forknum,
@@ -89,7 +92,7 @@ extern void smgrwrite(SMgrRelation reln, ForkNumber forknum,
BlockNumber blocknum, char *buffer, bool isTemp);
extern BlockNumber smgrnblocks(SMgrRelation reln, ForkNumber forknum);
extern void smgrtruncate(SMgrRelation reln, ForkNumber forknum,
- BlockNumber nblocks, bool isTemp);
+ BlockNumber nblocks);
extern void smgrimmedsync(SMgrRelation reln, ForkNumber forknum);
extern void smgrpreckpt(void);
extern void smgrsync(void);
@@ -114,7 +117,7 @@ extern void mdwrite(SMgrRelation reln, ForkNumber forknum,
BlockNumber blocknum, char *buffer, bool isTemp);
extern BlockNumber mdnblocks(SMgrRelation reln, ForkNumber forknum);
extern void mdtruncate(SMgrRelation reln, ForkNumber forknum,
- BlockNumber nblocks, bool isTemp);
+ BlockNumber nblocks);
extern void mdimmedsync(SMgrRelation reln, ForkNumber forknum);
extern void mdpreckpt(void);
extern void mdsync(void);
diff --git a/src/include/utils/rel.h b/src/include/utils/rel.h
index 444e892..da8a46b 100644
--- a/src/include/utils/rel.h
+++ b/src/include/utils/rel.h
@@ -347,7 +347,8 @@ typedef struct StdRdOptions
#define RelationOpenSmgr(relation) \
do { \
if ((relation)->rd_smgr == NULL) \
- smgrsetowner(&((relation)->rd_smgr), smgropen((relation)->rd_node)); \
+ smgrsetowner(&((relation)->rd_smgr), smgropen((relation)->rd_node, \
+ (relation)->rd_istemp)); \
} while (0)
/*
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
