To: PostgreSQL Hackers <pgsql-hackers@postgresql.org>
From: Konstantin Knizhnik <k.knizhnik@postgrespro.ru>
Subject: Bug in slot.c and are replication slots ever used at Window?
Message-ID: <9eb1a6d5-b66f-2640-598d-c5ea46b8f68a@postgrespro.ru>
Date: Thu, 30 Aug 2018 11:00:43 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.4.0
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Precedence: bulk

Hi hackers,

I am really confused.  If my conclusions are correct, then nobody ever 
tried to use replication slots at Windows!
The function RestoreSlotFromDisk in slot.c contains the following code:

static void
RestoreSlotFromDisk(const char *name)
{
     ReplicationSlotOnDisk cp;
     int            i;
     char        path[MAXPGPATH + 22];
     int            fd;
     bool        restored = false;
     int            readBytes;
     pg_crc32c    checksum;

     /* no need to lock here, no concurrent access allowed yet */

     /* delete temp file if it exists */
     sprintf(path, "pg_replslot/%s/state.tmp", name);
     if (unlink(path) < 0 && errno != ENOENT)
         ereport(PANIC,
                 (errcode_for_file_access(),
                  errmsg("could not remove file \"%s\": %m", path)));

     sprintf(path, "pg_replslot/%s/state", name);

     elog(DEBUG1, "restoring replication slot from \"%s\"", path);

     fd = OpenTransientFile(path, O_RDWR | PG_BINARY);

     /*
      * We do not need to handle this as we are rename()ing the 
directory into
      * place only after we fsync()ed the state file.
      */
     if (fd < 0)
         ereport(PANIC,
                 (errcode_for_file_access(),
                  errmsg("could not open file \"%s\": %m", path)));

     /*
      * Sync state file before we're reading from it. We might have crashed
      * while it wasn't synced yet and we shouldn't continue on that basis.
      */
pgstat_report_wait_start(WAIT_EVENT_REPLICATION_SLOT_RESTORE_SYNC);
     if (pg_fsync(fd) != 0)
     {
         int            save_errno = errno;

         CloseTransientFile(fd);
         errno = save_errno;
         ereport(PANIC,
                 (errcode_for_file_access(),
                  errmsg("could not fsync file \"%s\": %m",
                         path)));
     }
     pgstat_report_wait_end();

     /* Also sync the parent directory */
     START_CRIT_SECTION();
     fsync_fname(path, true);
     END_CRIT_SECTION();

     ...

Please notice that fsync_fname with comment "also sync parent directory" 
is called for path of the file!
fsync_fname in turn does the following:


   /*
  * fsync_fname -- Try to fsync a file or directory
  *
  * Ignores errors trying to open unreadable files, or trying to fsync
  * directories on systems where that isn't allowed/required. Reports
  * other errors non-fatally.
  */
int
fsync_fname(const char *fname, bool isdir, const char *progname)
{
     int            fd;
     int            flags;
     int            returncode;

     /*
      * Some OSs require directories to be opened read-only whereas other
      * systems don't allow us to fsync files opened read-only; so we 
need both
      * cases here.  Using O_RDWR will cause us to fail to fsync files 
that are
      * not writable by our userid, but we assume that's OK.
      */
     flags = PG_BINARY;
     if (!isdir)
         flags |= O_RDWR;
     else
         flags |= O_RDONLY;

     /*
      * Open the file, silently ignoring errors about unreadable files (or
      * unsupported operations, e.g. opening a directory under Windows), and
      * logging others.
      */
     fd = open(fname, flags);
     if (fd < 0)
     {
         if (errno == EACCES || (isdir && errno == EISDIR))
             return 0;
         fprintf(stderr, _("%s: could not open file \"%s\": %s\n"),
                 progname, fname, strerror(errno));
         return -1;
     }

     returncode = fsync(fd);


So if "isdir" is true (and it is true in this case), it sets O_RDONLY flag.
Then fsync_fname successfully opens slot file in readonly mode and calls 
fsync() which at windows
is substituted with _commit() which in turn is wrapper for FlushFileBuffers.
Finally FlushFileBuffers returns ERROR_ACCESS_DENINED which cause 
assertion failure in _commit:

if ( !FlushFileBuffers((HANDLE)_get_osfhandle(filedes)) ) {
              retval = GetLastError();
      }
      else {
              retval = 0;     /* return success */
      }

      /* map the OS return code to C errno value and return code */
      if (retval == 0)
              goto good;

      _doserrno = retval;

              }

      errno = EBADF;
      retval = -1;

      _ASSERTE(("Invalid file descriptor. File possibly closed by a different thread",0));


I think that the problem happen only with debug version of postgres.
Release version will just return error in this case which is silently ignored by RestoreSlotFromDisk function.

I think that bug fix is trivial: we just need to use fsync_parent_path instead of fsync_fname in RestoreSlotFromDisk.

-- 
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company