This is a multi-part message in MIME format.
--------------080403040604070408000809
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Tom Lane wrote:
> Magnus Hagander <magnus@[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 flu****ng 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
--------------080403040604070408000809
Content-Type: text/x-diff;
name="sync.diff"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
filename="sync.diff"
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 sup****t
*/
--- 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)
ere****t(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)
ere****t(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)
ere****t(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)
ere****t(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 sup****t
*/
! 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 sup****t
+ */
+ 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;
--------------080403040604070408000809
Content-Type: text/plain
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
--
Sent via pgsql-hackers mailing list (pgsql-hackers@[EMAIL PROTECTED]
)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
--------------080403040604070408000809--
|
