Re: [HACKERS] Fairly serious bug induced by latest guc enum changes
Tom Lane wrote: Magnus Hagander [EMAIL PROTECTED] writes: Not having looked at md.c (I confess...) but don't we have a problem in case we have closed the file without fsyncing it, and then change the fsync parameter? Well, we don't promise to retroactively fsync stuff we didn't before; and I wouldn't expect that to happen if I were changing the setting. What I *would* expect is that the system immediately starts to act according to the new setting, and that's not true as the code stands. As you say, the whole thing is pretty dubious from a data safety standpoint anyway. What I am concerned about here is people trying to compare performance measurements under different settings, and not being aware that the system's behavior doesn't change when they tell it to. Well, if they're running a performance measure that generates 16Mb data, I don't think they'll get very usable numbers anyway... //Magnus -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fairly serious bug induced by latest guc enum changes
Tom Lane wrote: Bruce Momjian [EMAIL PROTECTED] writes: Tom Lane wrote: No, my point was that there are three possible states of sync_bit and your patch only accounted for transitions between two of 'em. Did this every get addressed? I don't see a commit for it. I thought it got fixed here: 2008-05-14 10:02 mha * src/backend/access/transam/xlog.c: Remove the special variable for open_sync_bit used in O_SYNC and O_DSYNC modes, replacing it with a call to a function that derives it from the sync_method variable, now that it has distinct values for these two cases. This means that assign_xlog_sync_method() no longer changes any settings, thus fixing the bug introduced in the change to use a guc enum for wal_sync_method. Hmm ... or at least more or less fixed. Seems like there's no provision to close and reopen the file if enableFsync changes. Not sure if that's worth worrying about. We didn't have that before either, did we? We close it when the sync bit changes, but not just if we change say between fsync() and fdatasync(). Is there any actual reason we'd want to close it? //Magnus -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fairly serious bug induced by latest guc enum changes
Magnus Hagander [EMAIL PROTECTED] writes: Tom Lane wrote: Hmm ... or at least more or less fixed. Seems like there's no provision to close and reopen the file if enableFsync changes. Not sure if that's worth worrying about. We didn't have that before either, did we? No, so I think it's a pre-existing bug. We close it when the sync bit changes, but not just if we change say between fsync() and fdatasync(). Is there any actual reason we'd want to close it? The point is that if you turn the fsync GUC on or off while using a wal sync mode that requires supplying an option flag to open(), then really you ought to close the WAL file and re-open it with the new correct option flags. The fact that we're not doing that implies that the effects of a change in fsync might not fully take effect until the next WAL segment is started. Whether this is worth fixing isn't real clear. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fairly serious bug induced by latest guc enum changes
Tom Lane wrote: Magnus Hagander [EMAIL PROTECTED] writes: Tom Lane wrote: Hmm ... or at least more or less fixed. Seems like there's no provision to close and reopen the file if enableFsync changes. Not sure if that's worth worrying about. We didn't have that before either, did we? No, so I think it's a pre-existing bug. Ok, at least I'm reading the code right. We close it when the sync bit changes, but not just if we change say between fsync() and fdatasync(). Is there any actual reason we'd want to close it? The point is that if you turn the fsync GUC on or off while using a wal sync mode that requires supplying an option flag to open(), then really you ought to close the WAL file and re-open it with the new correct option flags. The fact that we're not doing that implies that the effects of a change in fsync might not fully take effect until the next WAL segment is started. Whether this is worth fixing isn't real clear. What scenario does it actually happen in, though? Doesn't the check: if (get_sync_bit(sync_method) != get_sync_bit(new_sync_method)) take care of that? If the sync bit changed, we close the file? Or are you talking about changing the variable fsync? If so, doesn't fsync=off also change the behavior of other parts of the code, so it's not just WAL, which means it'd be pretty unsafe *anyway* unless you actually sync the disks, and not just fsync? //Magnus //Magnus -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fairly serious bug induced by latest guc enum changes
Magnus Hagander [EMAIL PROTECTED] writes: Or are you talking about changing the variable fsync? If so, doesn't fsync=off also change the behavior of other parts of the code, so it's not just WAL, which means it'd be pretty unsafe *anyway* unless you actually sync the disks, and not just fsync? No, because the other uses of it are controlling whether to issue fsync() calls dynamically. The use in get_sync_bit is the only one that sets persistent state. In fact md.c goes out of its way to ensure that changing fsync on the fly behaves as expected. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fairly serious bug induced by latest guc enum changes
Tom Lane wrote: Magnus Hagander [EMAIL PROTECTED] writes: Or are you talking about changing the variable fsync? If so, doesn't fsync=off also change the behavior of other parts of the code, so it's not just WAL, which means it'd be pretty unsafe *anyway* unless you actually sync the disks, and not just fsync? No, because the other uses of it are controlling whether to issue fsync() calls dynamically. The use in get_sync_bit is the only one that sets persistent state. In fact md.c goes out of its way to ensure that changing fsync on the fly behaves as expected. Not having looked at md.c (I confess...) but don't we have a problem in case we have closed the file without fsyncing it, and then change the fsync parameter? Either way, I see your point, but I doubt it's worth getting upset over. Funning with fsync=off in the first place is bad, and if it takes you one WAL segment to recover, I think that's acceptable... //Magnus -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fairly serious bug induced by latest guc enum changes
Magnus Hagander [EMAIL PROTECTED] writes: Not having looked at md.c (I confess...) but don't we have a problem in case we have closed the file without fsyncing it, and then change the fsync parameter? Well, we don't promise to retroactively fsync stuff we didn't before; and I wouldn't expect that to happen if I were changing the setting. What I *would* expect is that the system immediately starts to act according to the new setting, and that's not true as the code stands. As you say, the whole thing is pretty dubious from a data safety standpoint anyway. What I am concerned about here is people trying to compare performance measurements under different settings, and not being aware that the system's behavior doesn't change when they tell it to. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fairly serious bug induced by latest guc enum changes
Tom Lane wrote: Magnus Hagander [EMAIL PROTECTED] writes: Right, but I still need the other part of the check, right? This one still fails the same check as my patch, no? Because I assume the hole you found there was that get_sync_bit() will return 0 for two different sync methods as long as none of them are O_SYNC or O_DSYNC... No, my point was that there are three possible states of sync_bit and your patch only accounted for transitions between two of 'em. For instance, if sync_bit goes to 0 we must close and reopen the file, else we'll be doing both O_SYNC flush and whatever flush method is supposed to be getting used. Did this every get addressed? I don't see a commit for it. -- Bruce Momjian [EMAIL PROTECTED]http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fairly serious bug induced by latest guc enum changes
Bruce Momjian [EMAIL PROTECTED] writes: Tom Lane wrote: No, my point was that there are three possible states of sync_bit and your patch only accounted for transitions between two of 'em. Did this every get addressed? I don't see a commit for it. I thought it got fixed here: 2008-05-14 10:02 mha * src/backend/access/transam/xlog.c: Remove the special variable for open_sync_bit used in O_SYNC and O_DSYNC modes, replacing it with a call to a function that derives it from the sync_method variable, now that it has distinct values for these two cases. This means that assign_xlog_sync_method() no longer changes any settings, thus fixing the bug introduced in the change to use a guc enum for wal_sync_method. Hmm ... or at least more or less fixed. Seems like there's no provision to close and reopen the file if enableFsync changes. Not sure if that's worth worrying about. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fairly serious bug induced by latest guc enum changes
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 - 1.307
--- xlog.c 13 May 2008 06:03:45 -
***
*** 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 |
Re: [HACKERS] Fairly serious bug induced by latest guc enum changes
Magnus Hagander [EMAIL PROTECTED] writes: 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. Okay, but you failed to correctly reproduce the conditions for 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? I don't think we care that much, since it's only invoked when we're about to do a moderately expensive kernel call. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fairly serious bug induced by latest guc enum changes
I wrote: Okay, but you failed to correctly reproduce the conditions for closing the old file. A more bulletproof solution might involve passing sync_method to get_sync_bit as an explicit parameter, and then the assign hook could do if (get_sync_bit(sync_method) != get_sync_bit(new_sync_method)) XLogFileClose(); regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fairly serious bug induced by latest guc enum changes
Tom Lane wrote: I wrote: Okay, but you failed to correctly reproduce the conditions for closing the old file. A more bulletproof solution might involve passing sync_method to get_sync_bit as an explicit parameter, and then the assign hook could do if (get_sync_bit(sync_method) != get_sync_bit(new_sync_method)) XLogFileClose(); Right, but I still need the other part of the check, right? This one still fails the same check as my patch, no? Because I assume the hole you found there was that get_sync_bit() will return 0 for two different sync methods as long as none of them are O_SYNC or O_DSYNC... //Magnus -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fairly serious bug induced by latest guc enum changes
Magnus Hagander [EMAIL PROTECTED] writes: Right, but I still need the other part of the check, right? This one still fails the same check as my patch, no? Because I assume the hole you found there was that get_sync_bit() will return 0 for two different sync methods as long as none of them are O_SYNC or O_DSYNC... No, my point was that there are three possible states of sync_bit and your patch only accounted for transitions between two of 'em. For instance, if sync_bit goes to 0 we must close and reopen the file, else we'll be doing both O_SYNC flush and whatever flush method is supposed to be getting used. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fairly serious bug induced by latest guc enum changes
Tom Lane wrote: I see that assign_xlog_sync_method() is still assigning to sync_method. This is 100% wrong: an assign hook is chartered to set derived values, but not to set the GUC variable itself. This will for example result in set_config_option() stacking the wrong value (the already-updated one) as the value to roll back to if the transaction aborts. Hm. I never quite did get my head around how the stacking work, that's probably why :-) You could just remove the assignment from assign_xlog_sync_method, although it might look a bit odd to be setting open_sync_bit but not sync_method. It also bothers me slightly that the derived and main values wouldn't be set at exactly the same point --- problems inside guc.c might lead to them getting out of sync. Another possibility is to stick with something equivalent to the former design: what GUC thinks is the variable is just a dummy static integer in guc.c, and the assign hook is still setting the real value that the rest of the code looks at. A minor advantage of this second way is that the real value could still be declared as enum rather than int. That value never was an enum, so that part is not an advantage. I still think going with the older method would be the safest, though, for the other reasons. You agree? Please fix this, and take another look at the prior WAL enum changes to see if the same problem hasn't been created elsewhere. (I assume you mean GUC enum here, that seems fairly obvious) The only other assign hooks are assign_syslog_facility and assign_session_replication_role. The changes there are: 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. //Magnus -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fairly serious bug induced by latest guc enum changes
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. (I assume you mean GUC enum here, that seems fairly obvious) Sorry, was writing in too much of a hurry. 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. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
