Thread: [HACKERS] pg_ls_waldir() & pg_ls_logdir()
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
On Mon, Feb 20, 2017 at 6:21 AM, Dave Page <dpage@pgadmin.org> 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
Hi On Wed, Mar 15, 2017 at 5:27 PM, Robert Haas <robertmhaas@gmail.com> wrote: > On Mon, Feb 20, 2017 at 6:21 AM, Dave Page <dpage@pgadmin.org> 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
On Thu, Mar 16, 2017 at 10:40 PM, Dave Page <dpage@pgadmin.org> 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
On Thu, Mar 16, 2017 at 9:54 AM, Thomas Munro <thomas.munro@enterprisedb.com> wrote: > On Thu, Mar 16, 2017 at 10:40 PM, Dave Page <dpage@pgadmin.org> 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
On Thu, Mar 16, 2017 at 6:09 AM, Dave Page <dpage@pgadmin.org> 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
On Thu, Mar 16, 2017 at 7:05 PM, Robert Haas <robertmhaas@gmail.com> wrote: > On Thu, Mar 16, 2017 at 6:09 AM, Dave Page <dpage@pgadmin.org> 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