From e24543ea7c4b2ac70c3d4b6df23e69f9cebf92f7 Mon Sep 17 00:00:00 2001 From: Thomas Munro Date: Mon, 25 Mar 2019 20:06:49 +1300 Subject: [PATCH 2/2] fixup --- src/backend/storage/smgr/md.c | 122 +++++++++++++++++++------------- src/backend/storage/sync/sync.c | 48 ++++--------- src/include/storage/md.h | 3 +- 3 files changed, 88 insertions(+), 85 deletions(-) diff --git a/src/backend/storage/smgr/md.c b/src/backend/storage/smgr/md.c index 8cc9fb16148..eced84f8413 100644 --- a/src/backend/storage/smgr/md.c +++ b/src/backend/storage/smgr/md.c @@ -874,54 +874,6 @@ mdimmedsync(SMgrRelation reln, ForkNumber forknum) } } -/* - * mdfilepath() - * - * Return the filename for the specified segment of the relation. The - * returned string is palloc'd. - */ -char * -mdfilepath(const FileTag *ftag) -{ - char *path, - *fullpath; - - /* - * We can safely pass InvalidBackendId as we never expect to sync - * any segments for temporary relations. - */ - path = GetRelationPath(ftag->rnode.dbNode, ftag->rnode.spcNode, - ftag->rnode.relNode, InvalidBackendId, ftag->forknum); - - if (ftag->segno > 0 && ftag->segno != InvalidSegmentNumber) - { - fullpath = psprintf("%s.%u", path, ftag->segno); - pfree(path); - } - else - fullpath = path; - - return fullpath; -} - -/* - * mdfiletagmatches() - * - * Returns true if the predicate tag matches with the file tag. - */ -bool -mdfiletagmatches(const FileTag *ftag, const FileTag *predicate, - SyncRequestType type) -{ - /* Today, we only do matching for hierarchy (forget database) requests */ - Assert(type == SYNC_FORGET_HIERARCHY_REQUEST); - - if (type == SYNC_FORGET_HIERARCHY_REQUEST) - return ftag->rnode.dbNode == predicate->rnode.dbNode; - - return false; -} - /* * register_dirty_segment() -- Mark a relation segment as needing fsync * @@ -1291,3 +1243,77 @@ _mdnblocks(SMgrRelation reln, ForkNumber forknum, MdfdVec *seg) /* note that this calculation will ignore any partial block at EOF */ return (BlockNumber) (len / BLCKSZ); } + +/* + * Sync a file to disk, given a file tag. Write the path into an output + * buffer so the caller can use it in error messages. + * + * Return 0 on success, and otherwise an errno value. + */ +int +mdsyncfiletag(const FileTag *ftag, char *path) +{ + SMgrRelation reln = smgropen(ftag->rnode, InvalidBackendId); + MdfdVec *v; + char *p; + + /* Provide the path for informational messages. */ + p = _mdfd_segpath(reln, ftag->forknum, ftag->segno); + strlcpy(path, p, MAXPGPATH); + pfree(p); + + /* Open the relation using the cache, for performance. */ + reln = smgropen(ftag->rnode, InvalidBackendId); + + /* Try to find open the requested segment. */ + v = _mdfd_openseg(reln, ftag->forknum, ftag->segno, 0); + if (v == NULL) + return ENOENT; + + /* Try to fsync the file. */ + if (FileSync(v->mdfd_vfd, WAIT_EVENT_DATA_FILE_SYNC) < 0) + return errno; + + return 0; +} + +/* + * Unlink a file, given a file tag. Write the path into an output + * buffer so the caller can use it in error messages. + * + * Return 0 on success, and otherwise an errno value. + */ +int +mdunlinkfiletag(const FileTag *ftag, char *path) +{ + SMgrRelation reln = smgropen(ftag->rnode, InvalidBackendId); + char *p; + + /* Compute the path. */ + p = _mdfd_segpath(reln, ftag->forknum, ftag->segno); + strlcpy(path, p, MAXPGPATH); + pfree(p); + + /* Try to unlink the file. */ + if (unlink(path) < 0) + return errno; + + return 0; +} + +/* + * Return true if the predicate tag matches with file tag, for the + * purpose of filtering out requests. + */ +bool +mdfiletagmatches(const FileTag *ftag, const FileTag *predicate, + SyncRequestType type) +{ + Assert(type == SYNC_FORGET_HIERARCHY_REQUEST); + + /* We only support dropping all requests for a given database. */ + if (type == SYNC_FORGET_HIERARCHY_REQUEST) + return ftag->rnode.dbNode == predicate->rnode.dbNode; + + return false; +} diff --git a/src/backend/storage/sync/sync.c b/src/backend/storage/sync/sync.c index 5f7db69e8a0..c82d2db91fb 100644 --- a/src/backend/storage/sync/sync.c +++ b/src/backend/storage/sync/sync.c @@ -82,18 +82,12 @@ static CycleCtr checkpoint_cycle_ctr = 0; #define UNLINKS_PER_ABSORB 10 /* - * This struct of function pointers defines the API between smgr.c and - * any individual storage manager module. Note that smgr subfunctions are - * generally expected to report problems via elog(ERROR). An exception is - * that smgr_unlink should use elog(WARNING), rather than erroring out, - * because we normally unlink relations during post-commit/abort cleanup, - * and so it's too late to raise an error. Also, various conditions that - * would normally be errors should be allowed during bootstrap and/or WAL - * recovery --- see comments in md.c for details. + * Function pointers for handling sync and unlink requests. */ typedef struct f_sync { - char* (*sync_filepath) (const FileTag *ftag); + int (*sync_syncfiletag) (const FileTag *ftag, char *path); + int (*sync_unlinkfiletag) (const FileTag *ftag, char *path); bool (*sync_filetagmatches) (const FileTag *ftag, const FileTag *predicate, SyncRequestType type); } f_sync; @@ -101,7 +95,8 @@ typedef struct f_sync static const f_sync syncsw[] = { /* magnetic disk */ { - .sync_filepath = mdfilepath, + .sync_syncfiletag = mdsyncfiletag, + .sync_unlinkfiletag = mdunlinkfiletag, .sync_filetagmatches = mdfiletagmatches } }; @@ -186,7 +181,7 @@ SyncPostCheckpoint(void) while (pendingUnlinks != NIL) { PendingUnlinkEntry *entry = (PendingUnlinkEntry *) linitial(pendingUnlinks); - char *path; + char path[MAXPGPATH]; /* * New entries are appended to the end, so if the entry is new we've @@ -201,9 +196,9 @@ SyncPostCheckpoint(void) break; /* Unlink the file */ - path = syncsw[entry->handler].sync_filepath(&(entry->ftag)); + errno = syncsw[entry->handler].sync_unlinkfiletag(&entry->ftag, path); - if (unlink(path) < 0) + if (errno != 0) { /* * There's a race condition, when the database is dropped at the @@ -217,7 +212,6 @@ SyncPostCheckpoint(void) (errcode_for_file_access(), errmsg("could not remove file \"%s\": %m", path))); } - pfree(path); /* And remove the list entry */ pendingUnlinks = list_delete_first(pendingUnlinks); @@ -374,15 +368,11 @@ ProcessSyncRequests(void) */ for (failures = 0; !(entry->canceled); failures++) { - char *path; - File fd; - - path = syncsw[entry->handler].sync_filepath(&(entry->ftag)); - fd = PathNameOpenFile(path, O_RDWR | PG_BINARY); + char path[MAXPGPATH]; INSTR_TIME_SET_CURRENT(sync_start); - if (fd >= 0 && - FileSync(fd, WAIT_EVENT_DATA_FILE_SYNC) >= 0) + errno = syncsw[entry->handler].sync_syncfiletag(&entry->ftag, path); + if (errno == 0) { /* Success; update statistics about sync timing */ INSTR_TIME_SET_CURRENT(sync_end); @@ -400,26 +390,14 @@ ProcessSyncRequests(void) path, (double) elapsed / 1000); - FileClose(fd); - pfree(path); break; /* out of retry loop */ } - /* Done with the file descriptor, close it */ - if (fd >= 0) - FileClose(fd); - /* * It is possible that the relation has been dropped or * truncated since the fsync request was entered. * Therefore, allow ENOENT, but only if we didn't fail - * already on this file. This applies both for - * smgrgetseg() and for FileSync, since fd.c might have - * closed the file behind our back. - * - * XXX is there any point in allowing more than one retry? - * Don't see one at the moment, but easy to change the - * test here if so. + * already on this file. */ if (!FILE_POSSIBLY_DELETED(errno) || failures > 0) ereport(data_sync_elevel(ERROR), @@ -432,8 +410,6 @@ ProcessSyncRequests(void) errmsg("could not fsync file \"%s\" but retrying: %m", path))); - pfree(path); - /* * Absorb incoming requests and check to see if a cancel * arrived for this relation fork. diff --git a/src/include/storage/md.h b/src/include/storage/md.h index fc13e34a6f6..b162f10edd9 100644 --- a/src/include/storage/md.h +++ b/src/include/storage/md.h @@ -44,7 +44,8 @@ extern void ForgetDatabaseSyncRequests(Oid dbid); extern void DropRelationFiles(RelFileNode *delrels, int ndelrels, bool isRedo); /* md sync callbacks */ -extern char *mdfilepath(const FileTag *ftag); +extern int mdsyncfiletag(const FileTag *ftag, char *path); +extern int mdunlinkfiletag(const FileTag *ftag, char *path); extern bool mdfiletagmatches(const FileTag *ftag, const FileTag *predicate, SyncRequestType type); -- 2.21.0