Re: [HACKERS] pg_ls_waldir() & pg_ls_logdir()

[Dave Page]

On Thu, Mar 16, 2017 at 7:05 PM, Robert Haas  wrote:
> On Thu, Mar 16, 2017 at 6:09 AM, Dave Page  wrote:
>> Hmm, good point. Google seems to be saying there isn't one. Patch
>> updated as you suggest (and I've added back in a function declaration
>> that got lost in the rebasing of the last version).
>
> OK, I took another look at this:
>
> - The documentation wasn't consistent with itself about the order in
> which the three columns were mentioned.  I changed it to say name,
> size, modification time both places and made the code also return the
> columns in that order.  And I renamed the columns to name, size, and
> modification, the last of which was chosen to match pg_stat_file().
>
> - I added an error check for the stat() call.
>
> - I moved the code to genfile.c where pg_ls_dir() already is; it seems
> to fit within the charter of that file.
>
> - I changed it to build a heap tuple directly instead of converting to
> text and then back to datums.  Seems less error-prone that way, and
> more consistent with what's done elsewhere in genfile.c.
>
> - I made it use a static-allocated buffer instead of a palloc'd one,
> just so it doesn't leak into the surrounding context.
>
> - I removed the function prototype and instead declared the helper
> function static.  If there's an intent to expose that function to
> extensions, the prototype should be in a header, not the .c file.
>
> - I adjusted the language in the documentation to be a bit more
> similar to what we've done elsewhere.
>
> With those changes, committed.

Thanks!

-- 
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pg_ls_waldir() & pg_ls_logdir()

[Robert Haas]

On Thu, Mar 16, 2017 at 6:09 AM, Dave Page  wrote:
> Hmm, good point. Google seems to be saying there isn't one. Patch
> updated as you suggest (and I've added back in a function declaration
> that got lost in the rebasing of the last version).

OK, I took another look at this:

- The documentation wasn't consistent with itself about the order in
which the three columns were mentioned.  I changed it to say name,
size, modification time both places and made the code also return the
columns in that order.  And I renamed the columns to name, size, and
modification, the last of which was chosen to match pg_stat_file().

- I added an error check for the stat() call.

- I moved the code to genfile.c where pg_ls_dir() already is; it seems
to fit within the charter of that file.

- I changed it to build a heap tuple directly instead of converting to
text and then back to datums.  Seems less error-prone that way, and
more consistent with what's done elsewhere in genfile.c.

- I made it use a static-allocated buffer instead of a palloc'd one,
just so it doesn't leak into the surrounding context.

- I removed the function prototype and instead declared the helper
function static.  If there's an intent to expose that function to
extensions, the prototype should be in a header, not the .c file.

- I adjusted the language in the documentation to be a bit more
similar to what we've done elsewhere.

With those changes, committed.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pg_ls_waldir() & pg_ls_logdir()

[Dave Page]

On Thu, Mar 16, 2017 at 9:54 AM, Thomas Munro
 wrote:
> On Thu, Mar 16, 2017 at 10:40 PM, Dave Page  wrote:
>>> +const int n = snprintf(NULL, 0, "%lld", attrib.st_size);
>
> I wonder what the portable printf directive is for off_t.  Maybe
> better to use INT64_FORMAT and cast to int64?

Hmm, good point. Google seems to be saying there isn't one. Patch
updated as you suggest (and I've added back in a function declaration
that got lost in the rebasing of the last version).

-- 
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index a521912317..e15ad77dec 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -19646,7 +19646,8 @@ postgres=# SELECT * FROM 
pg_walfile_name_offset(pg_stop_backup());
 database cluster directory and the log_directory can be
 accessed.  Use a relative path for files in the cluster directory,
 and a path matching the log_directory configuration setting
-for log files.  Use of these functions is restricted to superusers.
+for log files.  Use of these functions is restricted to superusers
+except where stated otherwise.

 

@@ -19669,6 +19670,26 @@ postgres=# SELECT * FROM 
pg_walfile_name_offset(pg_stop_backup());
   
   

+pg_ls_logdir()
+   
+   setof record
+   
+List the name, last modification time and size of files in the log 
directory.
+Execute permission may be granted to non-superuser roles.
+   
+  
+  
+   
+pg_ls_waldir()
+   
+   setof record
+   
+List the name, last modification time and size of files in the WAL 
directory.
+Execute permission may be granted to non-superuser roles.
+   
+  
+  
+   
 pg_read_file(filename text 
[, offset bigint, length bigint 
[, missing_ok boolean] ])

text
@@ -19699,7 +19720,7 @@ postgres=# SELECT * FROM 
pg_walfile_name_offset(pg_stop_backup());

 

-All of these functions take an optional missing_ok parameter,
+Some of these functions take an optional missing_ok 
parameter,
 which specifies the behavior when the file or directory does not exist.
 If true, the function returns NULL (except
 pg_ls_dir, which returns an empty result set). If
@@ -19720,6 +19741,26 @@ postgres=# SELECT * FROM 
pg_walfile_name_offset(pg_stop_backup());

 

+pg_ls_logdir
+   
+   
+pg_ls_logdir returns the last modified time (mtime), size
+and names of all the file in the log directory. By default only superusers
+can use this function, however additional roles may be granted execute
+permission if required.
+   
+
+   
+pg_ls_waldir
+   
+   
+pg_ls_waldir returns the last modified time (mtime), size
+and names of all the file in the write ahead log (WAL) directory. By
+default only superusers can use this function, however additional roles
+may be granted execute permission if required.
+   
+
+   
 pg_read_file


diff --git a/src/backend/catalog/system_views.sql 
b/src/backend/catalog/system_views.sql
index 0bce20914e..b6552da4b0 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -1102,3 +1102,6 @@ REVOKE EXECUTE ON FUNCTION pg_stat_reset() FROM public;
 REVOKE EXECUTE ON FUNCTION pg_stat_reset_shared(text) FROM public;
 REVOKE EXECUTE ON FUNCTION pg_stat_reset_single_table_counters(oid) FROM 
public;
 REVOKE EXECUTE ON FUNCTION pg_stat_reset_single_function_counters(oid) FROM 
public;
+
+REVOKE EXECUTE ON FUNCTION pg_ls_logdir() FROM public;
+REVOKE EXECUTE ON FUNCTION pg_ls_waldir() FROM public;
diff --git a/src/backend/utils/adt/misc.c b/src/backend/utils/adt/misc.c
index 1ec7f32470..ad75d18a2f 100644
--- a/src/backend/utils/adt/misc.c
+++ b/src/backend/utils/adt/misc.c
@@ -15,12 +15,14 @@
 #include "postgres.h"
 
 #include 
+#include 
 #include 
 #include 
 #include 
 #include 
 
 #include "access/sysattr.h"
+#include "access/xlog_internal.h"
 #include "catalog/pg_authid.h"
 #include "catalog/catalog.h"
 #include "catalog/pg_tablespace.h"
@@ -44,6 +46,8 @@
 #include "utils/builtins.h"
 #include "utils/timestamp.h"
 
+/* Generic function to return a directory listing of files */
+Datum pg_ls_dir_files(FunctionCallInfo fcinfo, char *dir);
 
 /*
  * Common subroutine for num_nulls() and num_nonnulls().
@@ -982,3 +986,111 @@ pg_current_logfile_1arg(PG_FUNCTION_ARGS)
 {
return pg_current_logfile(fcinfo);
 }
+
+
+typedef struct
+{
+   char*location;
+   DIR *dirdesc;
+} directory_fctx;
+
+/* Generic function to return a directory listing of files */
+Datum
+pg_ls_dir_files(FunctionCallInfo fcinfo, char *dir)
+{
+   FuncCallContext *funcctx;
+   struct dirent *de;
+   directory_fctx *fctx;
+
+  

Re: [HACKERS] pg_ls_waldir() & pg_ls_logdir()

[Thomas Munro]

On Thu, Mar 16, 2017 at 10:40 PM, Dave Page  wrote:
>> +const int n = snprintf(NULL, 0, "%lld", attrib.st_size);

I wonder what the portable printf directive is for off_t.  Maybe
better to use INT64_FORMAT and cast to int64?

-- 
Thomas Munro
http://www.enterprisedb.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pg_ls_waldir() & pg_ls_logdir()

[Dave Page]

Hi

On Wed, Mar 15, 2017 at 5:27 PM, Robert Haas  wrote:
> On Mon, Feb 20, 2017 at 6:21 AM, Dave Page  wrote:
>> Patch includes the code and doc updates.
>
> Review:
>
> +strftime(mtime, 25, "%Y-%m-%d %H:%M:%S %Z",
> localtime(&(attrib.st_ctime)));
> +const int n = snprintf(NULL, 0, "%lld", attrib.st_size);
> +char size[n+1];
> +snprintf(size, n+1, "%lld", attrib.st_size);
>
> We don't allow variable declarations in mid-block.  You've been
> programming in C++ for too long!

Err, yeah. Ooops. Fixed.

> The documentation should be updated to say that access to
> pg_ls_logdir() and pg_ls_waldir() can be granted via the permissions
> system (see the paragraph above the table you updated).
>
> The four existing functions in the documentation table each have a
> corresponding paragraph below the table, but the two new functions you
> added don't.

Added.

> +1 for the concept.

Thanks, updated patch attached.

-- 
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index a521912317..e15ad77dec 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -19646,7 +19646,8 @@ postgres=# SELECT * FROM 
pg_walfile_name_offset(pg_stop_backup());
 database cluster directory and the log_directory can be
 accessed.  Use a relative path for files in the cluster directory,
 and a path matching the log_directory configuration setting
-for log files.  Use of these functions is restricted to superusers.
+for log files.  Use of these functions is restricted to superusers
+except where stated otherwise.

 

@@ -19669,6 +19670,26 @@ postgres=# SELECT * FROM 
pg_walfile_name_offset(pg_stop_backup());
   
   

+pg_ls_logdir()
+   
+   setof record
+   
+List the name, last modification time and size of files in the log 
directory.
+Execute permission may be granted to non-superuser roles.
+   
+  
+  
+   
+pg_ls_waldir()
+   
+   setof record
+   
+List the name, last modification time and size of files in the WAL 
directory.
+Execute permission may be granted to non-superuser roles.
+   
+  
+  
+   
 pg_read_file(filename text 
[, offset bigint, length bigint 
[, missing_ok boolean] ])

text
@@ -19699,7 +19720,7 @@ postgres=# SELECT * FROM 
pg_walfile_name_offset(pg_stop_backup());

 

-All of these functions take an optional missing_ok parameter,
+Some of these functions take an optional missing_ok 
parameter,
 which specifies the behavior when the file or directory does not exist.
 If true, the function returns NULL (except
 pg_ls_dir, which returns an empty result set). If
@@ -19720,6 +19741,26 @@ postgres=# SELECT * FROM 
pg_walfile_name_offset(pg_stop_backup());

 

+pg_ls_logdir
+   
+   
+pg_ls_logdir returns the last modified time (mtime), size
+and names of all the file in the log directory. By default only superusers
+can use this function, however additional roles may be granted execute
+permission if required.
+   
+
+   
+pg_ls_waldir
+   
+   
+pg_ls_waldir returns the last modified time (mtime), size
+and names of all the file in the write ahead log (WAL) directory. By
+default only superusers can use this function, however additional roles
+may be granted execute permission if required.
+   
+
+   
 pg_read_file


diff --git a/src/backend/catalog/system_views.sql 
b/src/backend/catalog/system_views.sql
index 0bce20914e..b6552da4b0 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -1102,3 +1102,6 @@ REVOKE EXECUTE ON FUNCTION pg_stat_reset() FROM public;
 REVOKE EXECUTE ON FUNCTION pg_stat_reset_shared(text) FROM public;
 REVOKE EXECUTE ON FUNCTION pg_stat_reset_single_table_counters(oid) FROM 
public;
 REVOKE EXECUTE ON FUNCTION pg_stat_reset_single_function_counters(oid) FROM 
public;
+
+REVOKE EXECUTE ON FUNCTION pg_ls_logdir() FROM public;
+REVOKE EXECUTE ON FUNCTION pg_ls_waldir() FROM public;
diff --git a/src/backend/utils/adt/misc.c b/src/backend/utils/adt/misc.c
index 1ec7f32470..2fadad860b 100644
--- a/src/backend/utils/adt/misc.c
+++ b/src/backend/utils/adt/misc.c
@@ -15,12 +15,14 @@
 #include "postgres.h"
 
 #include 
+#include 
 #include 
 #include 
 #include 
 #include 
 
 #include "access/sysattr.h"
+#include "access/xlog_internal.h"
 #include "catalog/pg_authid.h"
 #include "catalog/catalog.h"
 #include "catalog/pg_tablespace.h"
@@ -982,3 +984,111 @@ pg_current_logfile_1arg(PG_FUNCTION_ARGS)
 {
return pg_current_logfile(fcinfo);
 }
+
+
+typedef struct
+{
+   char*location;
+   DIR *dirdesc;
+} directory_fctx;
+
+/* Generic function to 

Re: [HACKERS] pg_ls_waldir() & pg_ls_logdir()

[Robert Haas]

On Mon, Feb 20, 2017 at 6:21 AM, Dave Page  wrote:
> Patch includes the code and doc updates.

Review:

+strftime(mtime, 25, "%Y-%m-%d %H:%M:%S %Z",
localtime(&(attrib.st_ctime)));
+const int n = snprintf(NULL, 0, "%lld", attrib.st_size);
+char size[n+1];
+snprintf(size, n+1, "%lld", attrib.st_size);

We don't allow variable declarations in mid-block.  You've been
programming in C++ for too long!

The documentation should be updated to say that access to
pg_ls_logdir() and pg_ls_waldir() can be granted via the permissions
system (see the paragraph above the table you updated).

The four existing functions in the documentation table each have a
corresponding paragraph below the table, but the two new functions you
added don't.

+1 for the concept.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers