On 2016-05-02 22:00:14 +0200, Fabien COELHO wrote:
> I'm wondering that if _mdfd_openseg may return NULL, then ISTM that
> "opened_directly" should logically be false, because it was not opened?
>
> If so, maybe consider something like:
>
> opened_directy = (seg != NULL);
Hm, don't care either way. Seems just as valid to understand it as the
attempt to directly open the segment.
> Also, I do not understand why this issue is raised by the flushing patch, it
> seems rather independent.
It's not the flushing itself, it's 72a98a639574d2e25ed94652848555900c81a799
> >I'm not sure this is the best way to go about this. I can see valid
> >arguments for *always* using _mdfd_openseg() in mdsync(); and I'm
> >wondering whether we shouldn't make EXTENSION_* into a bitmask
> >(extend,extend_recovery,return_null,open_deleted).
>
> I thought about that when I looked at the previous fix, but it seemed that
> not all combinations made sense.
Sure, but that's nothing unusual. Here's an attempt at doing so - not
fully polished, just as a discussion point. I think it looks better.
Fabien, Robert, what do you think?
Greetings,
Andres Freund
diff --git a/src/backend/storage/smgr/md.c b/src/backend/storage/smgr/md.c
index 2981b41..f0c557f 100644
--- a/src/backend/storage/smgr/md.c
+++ b/src/backend/storage/smgr/md.c
@@ -163,23 +163,27 @@ static CycleCtr mdsync_cycle_ctr = 0;
static CycleCtr mdckpt_cycle_ctr = 0;
-typedef enum /* behavior for mdopen & _mdfd_getseg */
-{
- /* ereport if segment not present, create in recovery */
- EXTENSION_FAIL,
- /* return NULL if not present, create in recovery */
- EXTENSION_RETURN_NULL,
- /* return NULL if not present */
- EXTENSION_REALLY_RETURN_NULL,
- /* create new segments as needed */
- EXTENSION_CREATE
-} ExtensionBehavior;
+/*** behavior for mdopen & _mdfd_getseg ***/
+/* ereport if segment not present */
+#define EXTENSION_FAIL (1 << 0)
+/* return NULL if segment not present */
+#define EXTENSION_RETURN_NULL (1 << 1)
+/* create new segments as needed */
+#define EXTENSION_CREATE (1 << 2)
+/* create new segments if needed during recovery */
+#define EXTENSION_CREATE_RECOVERY (1 << 3)
+/*
+ * Allow opening deleted segments. Note that this is breaks mdnblocks() and
+ * related functionality - which currently is ok, because this is only
+ * required in the checkpointer which never uses mdnblocks().
+ */
+#define EXTENSION_OPEN_DELETED (1 << 4)
+
/* local routines */
static void mdunlinkfork(RelFileNodeBackend rnode, ForkNumber forkNum,
bool isRedo);
-static MdfdVec *mdopen(SMgrRelation reln, ForkNumber forknum,
- ExtensionBehavior behavior);
+static MdfdVec *mdopen(SMgrRelation reln, ForkNumber forknum, int behavior);
static void register_dirty_segment(SMgrRelation reln, ForkNumber forknum,
MdfdVec *seg);
static void register_unlink(RelFileNodeBackend rnode);
@@ -189,7 +193,7 @@ static char *_mdfd_segpath(SMgrRelation reln, ForkNumber forknum,
static MdfdVec *_mdfd_openseg(SMgrRelation reln, ForkNumber forkno,
BlockNumber segno, int oflags);
static MdfdVec *_mdfd_getseg(SMgrRelation reln, ForkNumber forkno,
- BlockNumber blkno, bool skipFsync, ExtensionBehavior behavior);
+ BlockNumber blkno, bool skipFsync, int behavior);
static BlockNumber _mdnblocks(SMgrRelation reln, ForkNumber forknum,
MdfdVec *seg);
@@ -570,7 +574,7 @@ mdextend(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum,
* invent one out of whole cloth.
*/
static MdfdVec *
-mdopen(SMgrRelation reln, ForkNumber forknum, ExtensionBehavior behavior)
+mdopen(SMgrRelation reln, ForkNumber forknum, int behavior)
{
MdfdVec *mdfd;
char *path;
@@ -596,8 +600,7 @@ mdopen(SMgrRelation reln, ForkNumber forknum, ExtensionBehavior behavior)
fd = PathNameOpenFile(path, O_RDWR | O_CREAT | O_EXCL | PG_BINARY, 0600);
if (fd < 0)
{
- if ((behavior == EXTENSION_RETURN_NULL ||
- behavior == EXTENSION_REALLY_RETURN_NULL) &&
+ if ((behavior & EXTENSION_RETURN_NULL) &&
FILE_POSSIBLY_DELETED(errno))
{
pfree(path);
@@ -691,7 +694,7 @@ mdwriteback(SMgrRelation reln, ForkNumber forknum,
segnum_end;
v = _mdfd_getseg(reln, forknum, blocknum, false,
- EXTENSION_REALLY_RETURN_NULL);
+ EXTENSION_RETURN_NULL);
/*
* We might be flushing buffers of already removed relations, that's
@@ -737,7 +740,8 @@ mdread(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum,
reln->smgr_rnode.node.relNode,
reln->smgr_rnode.backend);
- v = _mdfd_getseg(reln, forknum, blocknum, false, EXTENSION_FAIL);
+ v = _mdfd_getseg(reln, forknum, blocknum, false,
+ EXTENSION_FAIL | EXTENSION_CREATE_RECOVERY);
seekpos = (off_t) BLCKSZ *(blocknum % ((BlockNumber) RELSEG_SIZE));
@@ -812,7 +816,8 @@ mdwrite(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum,
reln->smgr_rnode.node.relNode,
reln->smgr_rnode.backend);
- v = _mdfd_getseg(reln, forknum, blocknum, skipFsync, EXTENSION_FAIL);
+ v = _mdfd_getseg(reln, forknum, blocknum, skipFsync,
+ EXTENSION_FAIL | EXTENSION_CREATE_RECOVERY);
seekpos = (off_t) BLCKSZ *(blocknum % ((BlockNumber) RELSEG_SIZE));
@@ -1219,7 +1224,9 @@ mdsync(void)
/* Attempt to open and fsync the target segment */
seg = _mdfd_getseg(reln, forknum,
(BlockNumber) segno * (BlockNumber) RELSEG_SIZE,
- false, EXTENSION_RETURN_NULL);
+ false,
+ EXTENSION_RETURN_NULL
+ | EXTENSION_OPEN_DELETED);
INSTR_TIME_SET_CURRENT(sync_start);
@@ -1773,14 +1780,18 @@ _mdfd_openseg(SMgrRelation reln, ForkNumber forknum, BlockNumber segno,
*/
static MdfdVec *
_mdfd_getseg(SMgrRelation reln, ForkNumber forknum, BlockNumber blkno,
- bool skipFsync, ExtensionBehavior behavior)
+ bool skipFsync, int behavior)
{
MdfdVec *v = mdopen(reln, forknum, behavior);
BlockNumber targetseg;
BlockNumber nextsegno;
+ /* some way to handle non-existant segments needs to be specified */
+ Assert(behavior &
+ (EXTENSION_FAIL | EXTENSION_CREATE | EXTENSION_RETURN_NULL));
+
if (!v)
- return NULL; /* if EXTENSION_(REALLY_)RETURN_NULL */
+ return NULL; /* if behavior & EXTENSION_RETURN_NULL */
targetseg = blkno / ((BlockNumber) RELSEG_SIZE);
for (nextsegno = 1; nextsegno <= targetseg; nextsegno++)
@@ -1795,8 +1806,8 @@ _mdfd_getseg(SMgrRelation reln, ForkNumber forknum, BlockNumber blkno,
if (nblocks > ((BlockNumber) RELSEG_SIZE))
elog(FATAL, "segment too big");
- if (behavior == EXTENSION_CREATE ||
- (InRecovery && behavior != EXTENSION_REALLY_RETURN_NULL))
+ if ((behavior & EXTENSION_CREATE) ||
+ (InRecovery && (behavior & EXTENSION_CREATE_RECOVERY)))
{
/*
* Normally we will create new segments only if authorized by
@@ -1827,15 +1838,16 @@ _mdfd_getseg(SMgrRelation reln, ForkNumber forknum, BlockNumber blkno,
}
flags = O_CREAT;
}
- else if (nblocks < ((BlockNumber) RELSEG_SIZE))
+ else if (!(behavior & EXTENSION_OPEN_DELETED) &&
+ nblocks < ((BlockNumber) RELSEG_SIZE))
{
/*
- * When not extending, only open the next segment if the
- * current one is exactly RELSEG_SIZE. If not (this branch),
- * either return NULL or fail.
+ * When not extending or explicitly opening deleted segments,
+ * only open the next segment if the current one is exactly
+ * RELSEG_SIZE. If not (this branch), either return NULL or
+ * fail.
*/
- if (behavior == EXTENSION_RETURN_NULL ||
- behavior == EXTENSION_REALLY_RETURN_NULL)
+ if (behavior & EXTENSION_RETURN_NULL)
{
/*
* Some callers discern between reasons for _mdfd_getseg()
@@ -1858,8 +1870,7 @@ _mdfd_getseg(SMgrRelation reln, ForkNumber forknum, BlockNumber blkno,
if (v->mdfd_chain == NULL)
{
- if ((behavior == EXTENSION_RETURN_NULL ||
- behavior == EXTENSION_REALLY_RETURN_NULL) &&
+ if ((behavior & EXTENSION_RETURN_NULL) &&
FILE_POSSIBLY_DELETED(errno))
return NULL;
ereport(ERROR,
--
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
