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

2017-03-17 Thread 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()

2017-03-16 Thread 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()

2017-03-16 Thread 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()

2017-03-16 Thread 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()

2017-03-16 Thread 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()

2017-03-15 Thread 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

[HACKERS] pg_ls_waldir() & pg_ls_logdir()

2017-02-20 Thread Dave Page 
Hi

Following various conversations on list and in person, including the
developer meeting in Brussels earlier this month, here is a patch that
implements pg_ls_logdir() and pg_ls_waldir() functions.

The ultimate aim of this (and followup work I'll be doing) is to
provide functionality to enable monitoring of PostgreSQL without
requiring a user with superuser permissions as many of us have users
for whom security policies prevent this or make it very difficult.

In order to achieve that, there are various pieces of functionality
such as pg_ls_dir() that need to have superuser checks removed to
allow permissions to be granted to a monitoring role. There were
objections in previous discussions to doing this with such generic
functions, hence this patch which adds two narrowly focussed functions
to allow tools to monitor the contents of the log and WAL directories.
Neither function has a hard-coded superuser check, but have ACLs that
prevent public execution by default.

Patch includes the code and doc updates.

-- 
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 d7738b18b7..ecd17a3528 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -19360,6 +19360,24 @@ 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.
+   
+  
+  
+   
+pg_ls_waldir()
+   
+   setof record
+   
+List the name, last modification time and size of files in the WAL 
directory.
+   
+  
+  
+   
 pg_read_file(filename text 
[, offset bigint, length bigint 
[, missing_ok boolean] ])

text
@@ -19390,7 +19408,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
diff --git a/src/backend/catalog/system_views.sql 
b/src/backend/catalog/system_views.sql
index 38be9cf1a0..4b67102439 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -1096,3 +1096,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 66d09bcb0c..a8cdf3bcbf 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"
@@ -46,6 +48,8 @@
 
 #define atooid(x)  ((Oid) strtoul((x), NULL, 10))
 
+/* 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().
@@ -892,3 +896,109 @@ parse_ident(PG_FUNCTION_ARGS)
 
PG_RETURN_DATUM(makeArrayResult(astate, CurrentMemoryContext));
 }
+
+
+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;
+
+   if (SRF_IS_FIRSTCALL())
+   {
+   MemoryContext oldcontext;
+   TupleDesc   tupdesc;
+
+   funcctx = SRF_FIRSTCALL_INIT();
+   oldcontext = 
MemoryContextSwitchTo(funcctx->multi_call_memory_ctx);
+
+   fctx = palloc(sizeof(directory_fctx));
+
+   tupdesc = CreateTemplateTupleDesc(3, false);
+   TupleDescInitEntry(tupdesc, (AttrNumber) 1, "mtime",
+  TIMESTAMPTZOID, -1, 0);
+   TupleDescInitEntry(tupdesc, (AttrNumber) 2, "size",
+  INT8OID, -1, 0);
+   TupleDescInitEntry(tupdesc, (AttrNumber) 3, "file",
+  TEXTOID, -1, 0);
+
+   funcctx->attinmeta =