Re: Add pg_file_sync() to adminpack

Fujii Masao <[email protected]>
Newsgroups gmane.comp.db.postgresql.devel.general
Message-ID <CAHGQGwE5n5+5erX1KkhHB8=FTRE+7gz5BD=ePektqXsJKgKtXw@mail.gmail.com> 
On Mon, Jan 6, 2020 at 3:42 PM Michael Paquier <[email protected]> wrote:
>
> On Mon, Jan 06, 2020 at 03:20:13PM +0900, Arthur Zakirov wrote:
> > It isn't case if a file doesn't exist. But if there are no permissions on
> > the file:
> >
> > PANIC:  could not open file "testfile": Permissions denied
> > server closed the connection unexpectedly
> >
> > It could be fixed by implementing a function like pg_file_sync_internal() or
> > by making the function fsync_fname_ext() external.
>
> The patch uses stat() to make sure that the file exists and has no
> issues.  Though it could be a problem with any kind of TOCTOU-like
> issues (looking at you, Windows, for ENOPERM), so I agree that it
> would make more sense to use pg_fsync() here with a fd opened first.

I agree that it's not good for pg_file_sync() to cause a PANIC.
I updated the patch so that pg_file_sync() uses fsync_fname_ext()
instead of fsync_fname() as Arthur suggested.

It's one of ideas to make pg_file_sync() open the file and directly call
pg_fsync(). But fsync_fname_ext() has already such code and
I'd like to avoid the code duplication.

Attached is the updated version of the patch.

Regards,

-- 
Fujii Masao
pg_file_sync_v2.patch (application/octet-stream, 7.7 KB) 
diff --git a/contrib/adminpack/Makefile b/contrib/adminpack/Makefile
index 9a464b11bc..630fea7726 100644
--- a/contrib/adminpack/Makefile
+++ b/contrib/adminpack/Makefile
@@ -7,7 +7,8 @@ OBJS = \
 PG_CPPFLAGS = -I$(libpq_srcdir)
 
 EXTENSION = adminpack
-DATA = adminpack--1.0.sql adminpack--1.0--1.1.sql adminpack--1.1--2.0.sql
+DATA = adminpack--1.0.sql adminpack--1.0--1.1.sql adminpack--1.1--2.0.sql\
+	adminpack--2.0--2.1.sql
 PGFILEDESC = "adminpack - support functions for pgAdmin"
 
 REGRESS = adminpack
diff --git a/contrib/adminpack/adminpack--2.0--2.1.sql b/contrib/adminpack/adminpack--2.0--2.1.sql
new file mode 100644
index 0000000000..b190d4f3ac
--- /dev/null
+++ b/contrib/adminpack/adminpack--2.0--2.1.sql
@@ -0,0 +1,17 @@
+/* contrib/adminpack/adminpack--2.0--2.1.sql */
+
+-- complain if script is sourced in psql, rather than via ALTER EXTENSION
+\echo Use "ALTER EXTENSION adminpack UPDATE TO '2.1'" to load this file. \quit
+
+/* ***********************************************
+ * Administrative functions for PostgreSQL
+ * *********************************************** */
+
+/* generic file access functions */
+
+CREATE OR REPLACE FUNCTION pg_catalog.pg_file_sync(text)
+RETURNS boolean
+AS 'MODULE_PATHNAME', 'pg_file_sync'
+LANGUAGE C VOLATILE STRICT;
+
+REVOKE EXECUTE ON FUNCTION pg_catalog.pg_file_sync(text) FROM PUBLIC;
diff --git a/contrib/adminpack/adminpack.c b/contrib/adminpack/adminpack.c
index 710f4ea32d..ba22be2022 100644
--- a/contrib/adminpack/adminpack.c
+++ b/contrib/adminpack/adminpack.c
@@ -43,6 +43,7 @@ PG_MODULE_MAGIC;
 
 PG_FUNCTION_INFO_V1(pg_file_write);
 PG_FUNCTION_INFO_V1(pg_file_write_v1_1);
+PG_FUNCTION_INFO_V1(pg_file_sync);
 PG_FUNCTION_INFO_V1(pg_file_rename);
 PG_FUNCTION_INFO_V1(pg_file_rename_v1_1);
 PG_FUNCTION_INFO_V1(pg_file_unlink);
@@ -215,6 +216,30 @@ pg_file_write_internal(text *file, text *data, bool replace)
 	return (count);
 }
 
+/* ------------------------------------
+ * pg_file_sync
+ *
+ * We REVOKE EXECUTE on the function from PUBLIC.
+ * Users can then grant access to it based on their policies.
+ */
+Datum
+pg_file_sync(PG_FUNCTION_ARGS)
+{
+	char	   *filename;
+	struct stat	fst;
+
+	filename = convert_and_check_filename(PG_GETARG_TEXT_PP(0), false);
+
+	if (stat(filename, &fst) < 0)
+		ereport(ERROR,
+				(errcode_for_file_access(),
+				 errmsg("could not stat file \"%s\": %m", filename)));
+
+	fsync_fname_ext(filename, S_ISDIR(fst.st_mode), false, ERROR);
+
+	PG_RETURN_BOOL(true);
+}
+
 /* ------------------------------------
  * pg_file_rename - old version
  *
diff --git a/contrib/adminpack/adminpack.control b/contrib/adminpack/adminpack.control
index 12569dcdd7..ae35d22156 100644
--- a/contrib/adminpack/adminpack.control
+++ b/contrib/adminpack/adminpack.control
@@ -1,6 +1,6 @@
 # adminpack extension
 comment = 'administrative functions for PostgreSQL'
-default_version = '2.0'
+default_version = '2.1'
 module_pathname = '$libdir/adminpack'
 relocatable = false
 schema = pg_catalog
diff --git a/contrib/adminpack/expected/adminpack.out b/contrib/adminpack/expected/adminpack.out
index 8747ac69a2..a72e29d657 100644
--- a/contrib/adminpack/expected/adminpack.out
+++ b/contrib/adminpack/expected/adminpack.out
@@ -56,6 +56,21 @@ RESET ROLE;
 REVOKE EXECUTE ON FUNCTION pg_file_write(text,text,bool) FROM regress_user1;
 REVOKE pg_read_all_settings FROM regress_user1;
 DROP ROLE regress_user1;
+-- sync
+SELECT pg_file_sync('test_file1'); -- sync file
+ pg_file_sync 
+--------------
+ t
+(1 row)
+
+SELECT pg_file_sync('global'); -- sync directory
+ pg_file_sync 
+--------------
+ t
+(1 row)
+
+SELECT pg_file_sync('test_file2'); -- not there
+ERROR:  could not stat file "test_file2": No such file or directory
 -- rename file
 SELECT pg_file_rename('test_file1', 'test_file2');
  pg_file_rename 
@@ -142,6 +157,8 @@ CREATE USER regress_user1;
 SET ROLE regress_user1;
 SELECT pg_file_write('test_file0', 'test0', false);
 ERROR:  permission denied for function pg_file_write
+SELECT pg_file_sync('test_file0');
+ERROR:  permission denied for function pg_file_sync
 SELECT pg_file_rename('test_file0', 'test_file0');
 ERROR:  permission denied for function pg_file_rename
 CONTEXT:  SQL function "pg_file_rename" statement 1
diff --git a/contrib/adminpack/sql/adminpack.sql b/contrib/adminpack/sql/adminpack.sql
index 1525f0a82b..6543fca8c1 100644
--- a/contrib/adminpack/sql/adminpack.sql
+++ b/contrib/adminpack/sql/adminpack.sql
@@ -29,6 +29,11 @@ REVOKE EXECUTE ON FUNCTION pg_file_write(text,text,bool) FROM regress_user1;
 REVOKE pg_read_all_settings FROM regress_user1;
 DROP ROLE regress_user1;
 
+-- sync
+SELECT pg_file_sync('test_file1'); -- sync file
+SELECT pg_file_sync('global'); -- sync directory
+SELECT pg_file_sync('test_file2'); -- not there
+
 -- rename file
 SELECT pg_file_rename('test_file1', 'test_file2');
 SELECT pg_read_file('test_file1');  -- not there
@@ -58,6 +63,7 @@ CREATE USER regress_user1;
 SET ROLE regress_user1;
 
 SELECT pg_file_write('test_file0', 'test0', false);
+SELECT pg_file_sync('test_file0');
 SELECT pg_file_rename('test_file0', 'test_file0');
 SELECT pg_file_unlink('test_file0');
 SELECT pg_logdir_ls();
diff --git a/doc/src/sgml/adminpack.sgml b/doc/src/sgml/adminpack.sgml
index 2655417366..f787a6d514 100644
--- a/doc/src/sgml/adminpack.sgml
+++ b/doc/src/sgml/adminpack.sgml
@@ -43,6 +43,13 @@
       Write, or append to, a text file
      </entry>
     </row>
+    <row>
+     <entry><function>pg_catalog.pg_file_sync(filename text)</function></entry>
+     <entry><type>boolean</type></entry>
+     <entry>
+      Sync a file or directory
+     </entry>
+    </row>
     <row>
      <entry><function>pg_catalog.pg_file_rename(oldname text, newname text <optional>, archivename text</optional>)</function></entry>
      <entry><type>boolean</type></entry>
@@ -79,6 +86,15 @@
   Returns the number of bytes written.
  </para>
 
+ <indexterm>
+  <primary>pg_file_sync</primary>
+ </indexterm>
+ <para>
+  <function>pg_file_sync</function> fsyncs the specified file or directory
+  named by <parameter>filename</parameter>.  Returns true on success,
+  an error is thrown otherwise (e.g., the specified file is not present).
+ </para>
+
  <indexterm>
   <primary>pg_file_rename</primary>
  </indexterm>
diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c
index fa79b45f63..b5f4df6a48 100644
--- a/src/backend/storage/file/fd.c
+++ b/src/backend/storage/file/fd.c
@@ -319,7 +319,6 @@ static void pre_sync_fname(const char *fname, bool isdir, int elevel);
 static void datadir_fsync_fname(const char *fname, bool isdir, int elevel);
 static void unlink_if_exists_fname(const char *fname, bool isdir, int elevel);
 
-static int	fsync_fname_ext(const char *fname, bool isdir, bool ignore_perm, int elevel);
 static int	fsync_parent_path(const char *fname, int elevel);
 
 
@@ -3376,7 +3375,7 @@ unlink_if_exists_fname(const char *fname, bool isdir, int elevel)
  *
  * Returns 0 if the operation succeeded, -1 otherwise.
  */
-static int
+int
 fsync_fname_ext(const char *fname, bool isdir, bool ignore_perm, int elevel)
 {
 	int			fd;
diff --git a/src/include/storage/fd.h b/src/include/storage/fd.h
index c6ce7eacf2..51e2ece3c9 100644
--- a/src/include/storage/fd.h
+++ b/src/include/storage/fd.h
@@ -145,6 +145,7 @@ extern int	pg_fsync_writethrough(int fd);
 extern int	pg_fdatasync(int fd);
 extern void pg_flush_data(int fd, off_t offset, off_t amount);
 extern void fsync_fname(const char *fname, bool isdir);
+extern int	fsync_fname_ext(const char *fname, bool isdir, bool ignore_perm, int elevel);
 extern int	durable_rename(const char *oldfile, const char *newfile, int loglevel);
 extern int	durable_unlink(const char *fname, int loglevel);
 extern int	durable_link_or_rename(const char *oldfile, const char *newfile, int loglevel);
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.