On 2020/01/17 13:36, Michael Paquier wrote:
On Thu, Jan 16, 2020 at 09:51:24AM -0500, Robert Haas wrote:
On Tue, Jan 14, 2020 at 10:08 AM Atsushi Torikoshi <ato...@gmail.com> wrote:
fails we can get error messages. So it seems straightforward for me to
'return true on success and emit an ERROR otherwise'.
I agree with reporting errors using ERROR, but it seems to me that we
ought to then make the function return 'void' rather than 'bool'.
Yeah, that should be either ERROR and return a void result, or issue a
WARNING/ERROR (depending on the code path, maybe PANIC?) with a
boolean status returned if there is a WARNING. Mixing both concepts
with an ERROR all the time and always a true status is just weird,
because you know that if no errors are raised then the status will be
always true. So there is no point to have a boolean status to begin
with.
OK, so our consensus is to return void on success and throw an error
otherwise. Attached is the updated version of the patch.
Regards,
--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters
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..1c6712e816
--- /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 void
+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..7b5a531e08 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_VOID();
+}
+
/* ------------------------------------
* 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..5738b0f6c4 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
+--------------
+
+(1 row)
+
+SELECT pg_file_sync('pg_stat'); -- sync directory
+ pg_file_sync
+--------------
+
+(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..918d0bdc65 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('pg_stat'); -- 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..21bd4e5dd5 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>void</type></entry>
+ <entry>
+ Flush a file or directory to disk
+ </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>. An error is thrown
+ on failure (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);
