Tom Lane wrote:
Magnus Hagander <[EMAIL PROTECTED]> writes:
I still think going with the older method would be the safest, though,
for the other reasons. You agree?
Seems reasonable to me.
Since it didn't really sound like a nice option, here's a third one I
came up with later. Basically, this one splits things apart so we only
use one variable, which is sync_method. Instead of using a macro to get
the open sync bit, it uses a function. This makes the assign hook only
responsible for flushing and closing the old file.
Thoughts? And if you like it, is it enough to expect the compiler to
figure out to inline it or should we explicitly inline it?
In these, the value was previously derived from a string and set in the
hook. It's now set directly by the GUC code, and the hook only updates
"other things" (setting the actual syslog facility, and resetting the
cache, respectively).
I think that means there are no bugs there.
Yeah, that's fine. I think though that I may have created a bug inside
GUC itself: the new stacking code could conceivably fail (palloc error)
between success return from the assign hook and setting up the stack
entry that is needed to undo the assignment on abort. In this situation
the assign hook would've made its "other thing" changes but there is no
GUC state to cause the hook to be called again to undo 'em. I need to
fix it so that any palloc'ing needed is done before calling the assign
hook.
Ok. I'll leave that to you for now :) I still need to figure out how the
stacking works because I want to add the "this setting came from file
X line Y" field to pg_settings, but that's a separate issue.
//Magnus
Index: xlog.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/access/transam/xlog.c,v
retrieving revision 1.307
diff -c -r1.307 xlog.c
*** xlog.c 12 May 2008 19:45:23 -0000 1.307
--- xlog.c 13 May 2008 06:03:45 -0000
***************
*** 86,97 ****
#define XLOGfileslop (2*CheckPointSegments + 1)
- /* these are derived from XLOG_sync_method by assign_xlog_sync_method */
- int sync_method = DEFAULT_SYNC_METHOD;
- static int open_sync_bit = DEFAULT_SYNC_FLAGBIT;
-
- #define XLOG_SYNC_BIT (enableFsync ? open_sync_bit : 0)
-
/*
* GUC support
*/
--- 86,91 ----
***************
*** 444,449 ****
--- 438,444 ----
static bool read_backup_label(XLogRecPtr *checkPointLoc,
XLogRecPtr *minRecoveryLoc);
static void rm_redo_error_callback(void *arg);
+ static int get_sync_bit(void);
/*
***************
*** 1960,1966 ****
*/
if (*use_existent)
{
! fd = BasicOpenFile(path, O_RDWR | PG_BINARY | XLOG_SYNC_BIT,
S_IRUSR | S_IWUSR);
if (fd < 0)
{
--- 1955,1961 ----
*/
if (*use_existent)
{
! fd = BasicOpenFile(path, O_RDWR | PG_BINARY | get_sync_bit(),
S_IRUSR | S_IWUSR);
if (fd < 0)
{
***************
*** 1986,1992 ****
unlink(tmppath);
! /* do not use XLOG_SYNC_BIT here --- want to fsync only at end of fill */
fd = BasicOpenFile(tmppath, O_RDWR | O_CREAT | O_EXCL | PG_BINARY,
S_IRUSR | S_IWUSR);
if (fd < 0)
--- 1981,1987 ----
unlink(tmppath);
! /* do not use get_sync_bit() here --- want to fsync only at end of fill */
fd = BasicOpenFile(tmppath, O_RDWR | O_CREAT | O_EXCL | PG_BINARY,
S_IRUSR | S_IWUSR);
if (fd < 0)
***************
*** 2064,2070 ****
*use_existent = false;
/* Now open original target segment (might not be file I just made) */
! fd = BasicOpenFile(path, O_RDWR | PG_BINARY | XLOG_SYNC_BIT,
S_IRUSR | S_IWUSR);
if (fd < 0)
ereport(ERROR,
--- 2059,2065 ----
*use_existent = false;
/* Now open original target segment (might not be file I just made) */
! fd = BasicOpenFile(path, O_RDWR | PG_BINARY | get_sync_bit(),
S_IRUSR | S_IWUSR);
if (fd < 0)
ereport(ERROR,
***************
*** 2115,2121 ****
unlink(tmppath);
! /* do not use XLOG_SYNC_BIT here --- want to fsync only at end of fill */
fd = BasicOpenFile(tmppath, O_RDWR | O_CREAT | O_EXCL | PG_BINARY,
S_IRUSR | S_IWUSR);
if (fd < 0)
--- 2110,2116 ----
unlink(tmppath);
! /* do not use get_sync_bit() here --- want to fsync only at end of fill */
fd = BasicOpenFile(tmppath, O_RDWR | O_CREAT | O_EXCL | PG_BINARY,
S_IRUSR | S_IWUSR);
if (fd < 0)
***************
*** 2297,2303 ****
XLogFilePath(path, ThisTimeLineID, log, seg);
! fd = BasicOpenFile(path, O_RDWR | PG_BINARY | XLOG_SYNC_BIT,
S_IRUSR | S_IWUSR);
if (fd < 0)
ereport(PANIC,
--- 2292,2298 ----
XLogFilePath(path, ThisTimeLineID, log, seg);
! fd = BasicOpenFile(path, O_RDWR | PG_BINARY | get_sync_bit(),
S_IRUSR | S_IWUSR);
if (fd < 0)
ereport(PANIC,
***************
*** 3650,3656 ****
unlink(tmppath);
! /* do not use XLOG_SYNC_BIT here --- want to fsync only at end of fill */
fd = BasicOpenFile(tmppath, O_RDWR | O_CREAT | O_EXCL,
S_IRUSR | S_IWUSR);
if (fd < 0)
--- 3645,3651 ----
unlink(tmppath);
! /* do not use get_sync_bit() here --- want to fsync only at end of fill */
fd = BasicOpenFile(tmppath, O_RDWR | O_CREAT | O_EXCL,
S_IRUSR | S_IWUSR);
if (fd < 0)
***************
*** 6328,6341 ****
/*
! * GUC support
*/
! bool
! assign_xlog_sync_method(int new_sync_method, bool doit, GucSource source)
{
! int new_sync_bit = 0;
! switch (new_sync_method)
{
/*
* Values for these sync options are defined even if they are not
--- 6323,6339 ----
/*
! * Return the (possible) sync flag used for opening a file, depending on the
! * value of the GUC wal_sync_method.
*/
! static int
! get_sync_bit(void)
{
! /* If fsync is disabled, never open in sync mode */
! if (!enableFsync)
! return 0;
! switch (sync_method)
{
/*
* Values for these sync options are defined even if they are not
***************
*** 6346,6362 ****
case SYNC_METHOD_FSYNC:
case SYNC_METHOD_FSYNC_WRITETHROUGH:
case SYNC_METHOD_FDATASYNC:
! new_sync_bit = 0;
! break;
#ifdef OPEN_SYNC_FLAG
case SYNC_METHOD_OPEN:
! new_sync_bit = OPEN_SYNC_FLAG;
! break;
#endif
#ifdef OPEN_DATASYNC_FLAG
case SYNC_METHOD_OPEN_DSYNC:
! new_sync_bit = OPEN_DATASYNC_FLAG;
! break;
#endif
default:
/*
--- 6344,6357 ----
case SYNC_METHOD_FSYNC:
case SYNC_METHOD_FSYNC_WRITETHROUGH:
case SYNC_METHOD_FDATASYNC:
! return 0;
#ifdef OPEN_SYNC_FLAG
case SYNC_METHOD_OPEN:
! return OPEN_SYNC_FLAG;
#endif
#ifdef OPEN_DATASYNC_FLAG
case SYNC_METHOD_OPEN_DSYNC:
! return OPEN_DATASYNC_FLAG;
#endif
default:
/*
***************
*** 6364,6377 ****
* new_sync_method are controlled by the available enum
* options.
*/
! elog(PANIC, "unrecognized wal_sync_method: %d", new_sync_method);
! break;
}
if (!doit)
return true;
! if (sync_method != new_sync_method || open_sync_bit != new_sync_bit)
{
/*
* To ensure that no blocks escape unsynced, force an fsync on the
--- 6359,6379 ----
* new_sync_method are controlled by the available enum
* options.
*/
! elog(PANIC, "unrecognized wal_sync_method: %d", sync_method);
! return 0; /* silence warning */
}
+ }
+ /*
+ * GUC support
+ */
+ bool
+ assign_xlog_sync_method(int new_sync_method, bool doit, GucSource source)
+ {
if (!doit)
return true;
! if (sync_method != new_sync_method)
{
/*
* To ensure that no blocks escape unsynced, force an fsync on the
***************
*** 6386,6396 ****
(errcode_for_file_access(),
errmsg("could not fsync log file %u, segment %u: %m",
openLogId, openLogSeg)));
! if (open_sync_bit != new_sync_bit)
XLogFileClose();
}
- sync_method = new_sync_method;
- open_sync_bit = new_sync_bit;
}
return true;
--- 6388,6397 ----
(errcode_for_file_access(),
errmsg("could not fsync log file %u, segment %u: %m",
openLogId, openLogSeg)));
! if ((sync_method == SYNC_METHOD_OPEN && new_sync_method == SYNC_METHOD_OPEN_DSYNC) ||
! (sync_method == SYNC_METHOD_OPEN_DSYNC && new_sync_method == SYNC_METHOD_OPEN))
XLogFileClose();
}
}
return true;
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
