Thanks for taking a look!
At Thu, 28 Jul 2022 16:22:17 -0400, Tom Lane <t...@sss.pgh.pa.us> wrote in
> Kyotaro Horiguchi <horikyota....@gmail.com> writes:
> > - Simplified the implementation (by complexifying argument handling..).
> > - REVOKEd EXECUTE from the new functions.
> > - Edited the signature of the two functions.
>
> >> - pg_read_file ( filename text [, offset bigint, length bigint [,
> >> missing_ok boolean ]] ) → text
> >> + pg_read_file ( filename text [, offset bigint, length bigint ] [,
> >> missing_ok boolean ] ) → text
>
> I'm okay with allowing this variant of the functions. Since there's
> no implicit cast between bigint and bool, plus the fact that you
> can't give just offset without length, there shouldn't be much risk
> of confusion as to which variant to invoke.
Grad to hear that.
> I don't really like the implementation style though. That mess of
> PG_NARGS tests is illegible code already and this makes it worse.
Ah..., I have to admit that I faintly felt that feeling while on it...
> I think it'd be way cleaner to have all the PG_GETARG calls in the
> bottom SQL-callable functions (which are already one-per-signature)
> and then pass them on to a common function that has an ordinary C
> call signature, along the lines of
>
> static Datum
> pg_read_file_common(text *filename_t,
> int64 seek_offset, int64 bytes_to_read,
> bool read_to_eof, bool missing_ok)
> {
> if (read_to_eof)
> bytes_to_read = -1; // or just Assert that it's -1 ?
I prefer assertion since that parameter cannot be passed by users.
> else if (bytes_to_read < 0)
> ereport(...);
> ...
> }
This function cannot return NULL directly. Without the ability to
return NULL, it is pointless for the function to return Datum. In the
attached the function returns text*.
> Datum
> pg_read_file_off_len(PG_FUNCTION_ARGS)
> {
> text *filename_t = PG_GETARG_TEXT_PP(0);
> int64 seek_offset = PG_GETARG_INT64(1);
> int64 bytes_to_read = PG_GETARG_INT64(2);
>
> return pg_read_file_common(filename_t, seek_offset, bytes_to_read,
> false, false);
As the result this function need to return NULL or TEXT_P according to
the returned value from pg_read_file_common.
+ if (!ret)
+ PG_RETURN_NULL();
+
+ PG_RETURN_TEXT_P(ret);
> }
# I'm tempted to call read_text_file() directly from each SQL functions..
Please find the attached. I added some regression tests for both
pg_read_file() and pg_read_binary_file().
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
>From 0edfc4f411b9b1fa5731c5f28d6225256f7077e1 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyota....@gmail.com>
Date: Thu, 30 Jun 2022 10:30:35 +0900
Subject: [PATCH v4] Add an argument variation to pg_read_file
Let the functions pg_read_file and pg_read_binary_file have the
argument variation of f(filename, missing_ok) so that the functions
can read the whole file tolerating the file to be missing.
---
doc/src/sgml/func.sgml | 4 +-
src/backend/catalog/system_functions.sql | 4 +
src/backend/utils/adt/genfile.c | 205 +++++++++++++------
src/include/catalog/pg_proc.dat | 11 +-
src/test/regress/expected/misc_functions.out | 77 +++++++
src/test/regress/sql/misc_functions.sql | 31 +++
6 files changed, 260 insertions(+), 72 deletions(-)
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 21f8ab73e2..49ebf6dea9 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -28636,7 +28636,7 @@ SELECT pg_size_pretty(sum(pg_relation_size(relid))) AS total_size
<indexterm>
<primary>pg_read_file</primary>
</indexterm>
- <function>pg_read_file</function> ( <parameter>filename</parameter> <type>text</type> <optional>, <parameter>offset</parameter> <type>bigint</type>, <parameter>length</parameter> <type>bigint</type> <optional>, <parameter>missing_ok</parameter> <type>boolean</type> </optional></optional> )
+ <function>pg_read_file</function> ( <parameter>filename</parameter> <type>text</type> <optional>, <parameter>offset</parameter> <type>bigint</type>, <parameter>length</parameter> <type>bigint</type> </optional> <optional>, <parameter>missing_ok</parameter> <type>boolean</type> </optional> )
<returnvalue>text</returnvalue>
</para>
<para>
@@ -28661,7 +28661,7 @@ SELECT pg_size_pretty(sum(pg_relation_size(relid))) AS total_size
<indexterm>
<primary>pg_read_binary_file</primary>
</indexterm>
- <function>pg_read_binary_file</function> ( <parameter>filename</parameter> <type>text</type> <optional>, <parameter>offset</parameter> <type>bigint</type>, <parameter>length</parameter> <type>bigint</type> <optional>, <parameter>missing_ok</parameter> <type>boolean</type> </optional></optional> )
+ <function>pg_read_binary_file</function> ( <parameter>filename</parameter> <type>text</type> <optional>, <parameter>offset</parameter> <type>bigint</type>, <parameter>length</parameter> <type>bigint</type> </optional> <optional>, <parameter>missing_ok</parameter> <type>boolean</type> </optional> )
<returnvalue>bytea</returnvalue>
</para>
<para>
diff --git a/src/backend/catalog/system_functions.sql b/src/backend/catalog/system_functions.sql
index 73da687d5d..30a048f6b0 100644
--- a/src/backend/catalog/system_functions.sql
+++ b/src/backend/catalog/system_functions.sql
@@ -659,12 +659,16 @@ REVOKE EXECUTE ON FUNCTION pg_ls_tmpdir(oid) FROM public;
REVOKE EXECUTE ON FUNCTION pg_read_file(text) FROM public;
+REVOKE EXECUTE ON FUNCTION pg_read_file(text,boolean) FROM public;
+
REVOKE EXECUTE ON FUNCTION pg_read_file(text,bigint,bigint) FROM public;
REVOKE EXECUTE ON FUNCTION pg_read_file(text,bigint,bigint,boolean) FROM public;
REVOKE EXECUTE ON FUNCTION pg_read_binary_file(text) FROM public;
+REVOKE EXECUTE ON FUNCTION pg_read_binary_file(text,boolean) FROM public;
+
REVOKE EXECUTE ON FUNCTION pg_read_binary_file(text,bigint,bigint) FROM public;
REVOKE EXECUTE ON FUNCTION pg_read_binary_file(text,bigint,bigint,boolean) FROM public;
diff --git a/src/backend/utils/adt/genfile.c b/src/backend/utils/adt/genfile.c
index 2bf5219256..7ac6ede42d 100644
--- a/src/backend/utils/adt/genfile.c
+++ b/src/backend/utils/adt/genfile.c
@@ -278,81 +278,49 @@ pg_read_file(PG_FUNCTION_ARGS)
*
* No superuser check done here- instead privileges are handled by the
* GRANT system.
+ *
+ * If read_to_eof is true, bytes_to_read must be -1, otherwise negative values
+ * are not allowed for bytes_to_read.
*/
-Datum
-pg_read_file_v2(PG_FUNCTION_ARGS)
+static text*
+pg_read_file_common(text *filename_t, int64 seek_offset, int64 bytes_to_read,
+ bool read_to_eof, bool missing_ok)
{
- text *filename_t = PG_GETARG_TEXT_PP(0);
- int64 seek_offset = 0;
- int64 bytes_to_read = -1;
- bool missing_ok = false;
- char *filename;
- text *result;
-
- /* handle optional arguments */
- if (PG_NARGS() >= 3)
- {
- seek_offset = PG_GETARG_INT64(1);
- bytes_to_read = PG_GETARG_INT64(2);
-
- if (bytes_to_read < 0)
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("requested length cannot be negative")));
- }
- if (PG_NARGS() >= 4)
- missing_ok = PG_GETARG_BOOL(3);
-
- filename = convert_and_check_filename(filename_t);
-
- result = read_text_file(filename, seek_offset, bytes_to_read, missing_ok);
- if (result)
- PG_RETURN_TEXT_P(result);
- else
- PG_RETURN_NULL();
+ if (read_to_eof)
+ Assert(bytes_to_read == -1);
+ else if (bytes_to_read < 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("requested length cannot be negative")));
+
+ return read_text_file(convert_and_check_filename(filename_t),
+ seek_offset, bytes_to_read, missing_ok);
}
/*
- * Read a section of a file, returning it as bytea
+ * Read a section of a file, returning it as bytea. Parameters are interpreted
+ * and restricted the same with pg_read_file_common().
*/
-Datum
-pg_read_binary_file(PG_FUNCTION_ARGS)
+static bytea*
+pg_read_binary_file_common(text *filename_t,
+ int64 seek_offset, int64 bytes_to_read,
+ bool read_to_eof, bool missing_ok)
{
- text *filename_t = PG_GETARG_TEXT_PP(0);
- int64 seek_offset = 0;
- int64 bytes_to_read = -1;
- bool missing_ok = false;
- char *filename;
- bytea *result;
-
- /* handle optional arguments */
- if (PG_NARGS() >= 3)
- {
- seek_offset = PG_GETARG_INT64(1);
- bytes_to_read = PG_GETARG_INT64(2);
-
- if (bytes_to_read < 0)
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("requested length cannot be negative")));
- }
- if (PG_NARGS() >= 4)
- missing_ok = PG_GETARG_BOOL(3);
-
- filename = convert_and_check_filename(filename_t);
-
- result = read_binary_file(filename, seek_offset,
- bytes_to_read, missing_ok);
- if (result)
- PG_RETURN_BYTEA_P(result);
- else
- PG_RETURN_NULL();
+ if (read_to_eof)
+ Assert(bytes_to_read == -1);
+ else if (bytes_to_read < 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("requested length cannot be negative")));
+
+ return read_binary_file(convert_and_check_filename(filename_t),
+ seek_offset, bytes_to_read, missing_ok);
}
/*
- * Wrapper functions for the 1 and 3 argument variants of pg_read_file_v2()
- * and pg_read_binary_file().
+ * Wrapper functions for the variants of SQL functions pg_read_file() and
+ * pg_read_binary_file().
*
* These are necessary to pass the sanity check in opr_sanity, which checks
* that all built-in functions that share the implementing C function take
@@ -361,25 +329,126 @@ pg_read_binary_file(PG_FUNCTION_ARGS)
Datum
pg_read_file_off_len(PG_FUNCTION_ARGS)
{
- return pg_read_file_v2(fcinfo);
+ text *filename_t = PG_GETARG_TEXT_PP(0);
+ int64 seek_offset = PG_GETARG_INT64(1);
+ int64 bytes_to_read = PG_GETARG_INT64(2);
+ text *ret;
+
+ ret = pg_read_file_common(filename_t, seek_offset, bytes_to_read,
+ false, false);
+ if (!ret)
+ PG_RETURN_NULL();
+
+ PG_RETURN_TEXT_P(ret);
+}
+
+Datum
+pg_read_file_off_len_missing(PG_FUNCTION_ARGS)
+{
+ text *filename_t = PG_GETARG_TEXT_PP(0);
+ int64 seek_offset = PG_GETARG_INT64(1);
+ int64 bytes_to_read = PG_GETARG_INT64(2);
+ bool missing_ok = PG_GETARG_BOOL(3);
+ text *ret;
+
+ ret = pg_read_file_common(filename_t, seek_offset, bytes_to_read,
+ false, missing_ok);
+
+ if (!ret)
+ PG_RETURN_NULL();
+
+ PG_RETURN_TEXT_P(ret);
}
Datum
pg_read_file_all(PG_FUNCTION_ARGS)
{
- return pg_read_file_v2(fcinfo);
+ text *filename_t = PG_GETARG_TEXT_PP(0);
+ text *ret;
+
+ ret = pg_read_file_common(filename_t, 0, -1, true, false);
+
+ if (!ret)
+ PG_RETURN_NULL();
+
+ PG_RETURN_TEXT_P(ret);
+}
+
+Datum
+pg_read_file_all_missing(PG_FUNCTION_ARGS)
+{
+ text *filename_t = PG_GETARG_TEXT_PP(0);
+ bool missing_ok = PG_GETARG_BOOL(1);
+ text *ret;
+
+ ret = pg_read_file_common(filename_t, 0, -1, true, missing_ok);
+
+ if (!ret)
+ PG_RETURN_NULL();
+
+ PG_RETURN_TEXT_P(ret);
}
Datum
pg_read_binary_file_off_len(PG_FUNCTION_ARGS)
{
- return pg_read_binary_file(fcinfo);
+ text *filename_t = PG_GETARG_TEXT_PP(0);
+ int64 seek_offset = PG_GETARG_INT64(1);
+ int64 bytes_to_read = PG_GETARG_INT64(2);
+ text *ret;
+
+ ret = pg_read_binary_file_common(filename_t, seek_offset, bytes_to_read,
+ false, false);
+ if (!ret)
+ PG_RETURN_NULL();
+
+ PG_RETURN_BYTEA_P(ret);
+}
+
+Datum
+pg_read_binary_file_off_len_missing(PG_FUNCTION_ARGS)
+{
+ text *filename_t = PG_GETARG_TEXT_PP(0);
+ int64 seek_offset = PG_GETARG_INT64(1);
+ int64 bytes_to_read = PG_GETARG_INT64(2);
+ bool missing_ok = PG_GETARG_BOOL(3);
+ text *ret;
+
+ ret = pg_read_binary_file_common(filename_t, seek_offset, bytes_to_read,
+ false, missing_ok);
+ if (!ret)
+ PG_RETURN_NULL();
+
+ PG_RETURN_BYTEA_P(ret);
}
Datum
pg_read_binary_file_all(PG_FUNCTION_ARGS)
{
- return pg_read_binary_file(fcinfo);
+ text *filename_t = PG_GETARG_TEXT_PP(0);
+ text *ret;
+
+ ret = pg_read_binary_file_common(filename_t, 0, -1, true, false);
+
+ if (!ret)
+ PG_RETURN_NULL();
+
+ PG_RETURN_BYTEA_P(ret);
+}
+
+Datum
+pg_read_binary_file_all_missing(PG_FUNCTION_ARGS)
+{
+ text *filename_t = PG_GETARG_TEXT_PP(0);
+ bool missing_ok = PG_GETARG_BOOL(1);
+ text *ret;
+
+ ret = pg_read_binary_file_common(filename_t, 0, -1, true, missing_ok);
+
+ if (!ret)
+ PG_RETURN_NULL();
+
+ PG_RETURN_BYTEA_P(ret);
}
/*
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 2e41f4d9e8..63392d24ff 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -6415,7 +6415,7 @@
proargtypes => 'text int8 int8', prosrc => 'pg_read_file_off_len' },
{ oid => '3293', descr => 'read text from a file',
proname => 'pg_read_file', provolatile => 'v', prorettype => 'text',
- proargtypes => 'text int8 int8 bool', prosrc => 'pg_read_file_v2' },
+ proargtypes => 'text int8 int8 bool', prosrc => 'pg_read_file_off_len_missing' },
{ oid => '4100',
descr => 'read text from a file - old version for adminpack 1.0',
proname => 'pg_read_file_old', provolatile => 'v', prorettype => 'text',
@@ -6423,15 +6423,22 @@
{ oid => '3826', descr => 'read text from a file',
proname => 'pg_read_file', provolatile => 'v', prorettype => 'text',
proargtypes => 'text', prosrc => 'pg_read_file_all' },
+{ oid => '8025', descr => 'read text from a file',
+ proname => 'pg_read_file', provolatile => 'v', prorettype => 'text',
+ proargtypes => 'text bool', prosrc => 'pg_read_file_all_missing' },
{ oid => '3827', descr => 'read bytea from a file',
proname => 'pg_read_binary_file', provolatile => 'v', prorettype => 'bytea',
proargtypes => 'text int8 int8', prosrc => 'pg_read_binary_file_off_len' },
{ oid => '3295', descr => 'read bytea from a file',
proname => 'pg_read_binary_file', provolatile => 'v', prorettype => 'bytea',
- proargtypes => 'text int8 int8 bool', prosrc => 'pg_read_binary_file' },
+ proargtypes => 'text int8 int8 bool',
+ prosrc => 'pg_read_binary_file_off_len_missing' },
{ oid => '3828', descr => 'read bytea from a file',
proname => 'pg_read_binary_file', provolatile => 'v', prorettype => 'bytea',
proargtypes => 'text', prosrc => 'pg_read_binary_file_all' },
+{ oid => '8026', descr => 'read bytea from a file',
+ proname => 'pg_read_binary_file', provolatile => 'v', prorettype => 'bytea',
+ proargtypes => 'text bool', prosrc => 'pg_read_binary_file_all_missing' },
{ oid => '2625', descr => 'list all files in a directory',
proname => 'pg_ls_dir', prorows => '1000', proretset => 't',
provolatile => 'v', prorettype => 'text', proargtypes => 'text',
diff --git a/src/test/regress/expected/misc_functions.out b/src/test/regress/expected/misc_functions.out
index 01d1ad0b9a..07e9348f97 100644
--- a/src/test/regress/expected/misc_functions.out
+++ b/src/test/regress/expected/misc_functions.out
@@ -372,6 +372,83 @@ select count(*) >= 0 as ok from pg_ls_archive_statusdir();
t
(1 row)
+-- pg_read_file()
+\pset null <null>
+select substr(pg_read_file('pg_hba.conf'), 1, 30);
+ substr
+--------------------------------
+ # PostgreSQL Client Authentica
+(1 row)
+
+select pg_read_file('pg_hba.conf', 0, 30);
+ pg_read_file
+--------------------------------
+ # PostgreSQL Client Authentica
+(1 row)
+
+-- Test missing_ok
+select pg_read_file('does not exist'); -- error
+ERROR: could not open file "does not exist" for reading: No such file or directory
+select pg_read_file('does not exist', false); -- error
+ERROR: could not open file "does not exist" for reading: No such file or directory
+select pg_read_file('does not exist', 0, 30, false); -- error
+ERROR: could not open file "does not exist" for reading: No such file or directory
+select pg_read_file('does not exist', true); -- ok
+ pg_read_file
+--------------
+ <null>
+(1 row)
+
+select pg_read_file('does not exist', 0, 30, true); -- ok
+ pg_read_file
+--------------
+ <null>
+(1 row)
+
+-- Test invalid argument
+select pg_read_file('does not exist', 0, -1); -- error
+ERROR: requested length cannot be negative
+select pg_read_file('does not exist', 0, -1, true); -- error
+ERROR: requested length cannot be negative
+-- pg_read_binary_file()
+select substr(pg_read_binary_file('pg_hba.conf'), 1, 30);
+ substr
+----------------------------------------------------------------
+ \x2320506f737467726553514c20436c69656e742041757468656e74696361
+(1 row)
+
+select pg_read_binary_file('pg_hba.conf', 0, 30);
+ pg_read_binary_file
+----------------------------------------------------------------
+ \x2320506f737467726553514c20436c69656e742041757468656e74696361
+(1 row)
+
+-- Test missing_ok
+select pg_read_binary_file('does not exist'); -- error
+ERROR: could not open file "does not exist" for reading: No such file or directory
+select pg_read_binary_file('does not exist', false); -- error
+ERROR: could not open file "does not exist" for reading: No such file or directory
+select pg_read_binary_file('does not exist', 0, 30, false); -- error
+ERROR: could not open file "does not exist" for reading: No such file or directory
+select pg_read_binary_file('does not exist', true); -- ok
+ pg_read_binary_file
+---------------------
+ <null>
+(1 row)
+
+select pg_read_binary_file('does not exist', 0, 30, true); -- ok
+ pg_read_binary_file
+---------------------
+ <null>
+(1 row)
+
+-- Test invalid argument
+select pg_read_binary_file('does not exist', 0, -1); -- error
+ERROR: requested length cannot be negative
+select pg_read_binary_file('does not exist', 0, -1, true); -- error
+ERROR: requested length cannot be negative
+\pset null
+-- pg_ls_dir()
select * from (select pg_ls_dir('.') a) a where a = 'base' limit 1;
a
------
diff --git a/src/test/regress/sql/misc_functions.sql b/src/test/regress/sql/misc_functions.sql
index 072fc36a1f..8f5251abbb 100644
--- a/src/test/regress/sql/misc_functions.sql
+++ b/src/test/regress/sql/misc_functions.sql
@@ -123,6 +123,35 @@ from (select pg_ls_waldir() w) ss where length((w).name) = 24 limit 1;
select count(*) >= 0 as ok from pg_ls_archive_statusdir();
+-- pg_read_file()
+\pset null <null>
+select substr(pg_read_file('pg_hba.conf'), 1, 30);
+select pg_read_file('pg_hba.conf', 0, 30);
+-- Test missing_ok
+select pg_read_file('does not exist'); -- error
+select pg_read_file('does not exist', false); -- error
+select pg_read_file('does not exist', 0, 30, false); -- error
+select pg_read_file('does not exist', true); -- ok
+select pg_read_file('does not exist', 0, 30, true); -- ok
+-- Test invalid argument
+select pg_read_file('does not exist', 0, -1); -- error
+select pg_read_file('does not exist', 0, -1, true); -- error
+
+-- pg_read_binary_file()
+select substr(pg_read_binary_file('pg_hba.conf'), 1, 30);
+select pg_read_binary_file('pg_hba.conf', 0, 30);
+-- Test missing_ok
+select pg_read_binary_file('does not exist'); -- error
+select pg_read_binary_file('does not exist', false); -- error
+select pg_read_binary_file('does not exist', 0, 30, false); -- error
+select pg_read_binary_file('does not exist', true); -- ok
+select pg_read_binary_file('does not exist', 0, 30, true); -- ok
+-- Test invalid argument
+select pg_read_binary_file('does not exist', 0, -1); -- error
+select pg_read_binary_file('does not exist', 0, -1, true); -- error
+\pset null
+
+-- pg_ls_dir()
select * from (select pg_ls_dir('.') a) a where a = 'base' limit 1;
-- Test missing_ok (second argument)
select pg_ls_dir('does not exist', false, false); -- error
@@ -133,6 +162,8 @@ select count(*) = 1 as dot_found
select count(*) = 1 as dot_found
from pg_ls_dir('.', false, false) as ls where ls = '.';
+
+
select * from (select (pg_timezone_names()).name) ptn where name='UTC' limit 1;
select count(*) > 0 from
--
2.31.1
