Re: Fairly serious bug induced by latest guc enum changes - Mailing list pgsql-hackers

Tom Lane wrote:
> Magnus Hagander <magnus@hagander.net> 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;