Обсуждение: pg_ls_tmpdir()

Hi hackers,

Attached is a patch to add a pg_ls_tmpdir() function that lists the
contents of a specified tablespace's pgsql_tmp directory.  This is
very similar to the existing pg_ls_logdir() and pg_ls_waldir()
functions.

By providing more visibility into the temporary file directories,
users can more easily track queries that are consuming a lot of disk
space for temporary files.  This function already provides enough
information to calculate the total temporary file usage per PID, but
it might be worthwhile to create a system view that does this, too.

I am submitting this patch for consideration in commitfest 2018-11.

Nathan

Bossart, Nathan wrote:
> Attached is a patch to add a pg_ls_tmpdir() function that lists the
> contents of a specified tablespace's pgsql_tmp directory.  This is
> very similar to the existing pg_ls_logdir() and pg_ls_waldir()
> functions.
> 
> By providing more visibility into the temporary file directories,
> users can more easily track queries that are consuming a lot of disk
> space for temporary files.  This function already provides enough
> information to calculate the total temporary file usage per PID, but
> it might be worthwhile to create a system view that does this, too.
> 
> I am submitting this patch for consideration in commitfest 2018-11.

The patch applies, builds without warning and passes "make check-world".

Since pgsql_tmp is only created when the first temp file is written,
calling the function on a newly initdb'ed data directory fails with

ERROR:  could not open directory "base/pgsql_tmp": No such file or directory

This should be fixed; it shouldn't be an error.

Calling the function with a non-existing tablespace OID produces:

ERROR:  could not open directory "pg_tblspc/1665/PG_12_201809121/pgsql_tmp": No such file or directory

Wouldn't it be better to have a check if the tablespace exists?

About the code:
The catversion bump shouldn't be part of the patch, it will
rot too quickly.  The committer is supposed to add it.


I think the idea to have such a function is fine, but I have two doubts:

1. Do we really need two functions, one without input argument
   to list the default tablespace?
   I think that anybody who uses with such a function whould
   be able to feed the OID of "pg_default".

2. There already are functions "pg_ls_logdir" and "pg_ls_waldir",
   and I can imagine new requests, e.g. pg_ls_datafiles() to list the
   data files in a database directory.

   It may make sense to have a generic function like

      pg_ls_dir(dirname text, tablespace oid)

   instead.  But maybe that's taking it too far...

I'll set the patch to "waiting for author".

Yours,
Laurenz Albe

On Wed, Sep 26, 2018 at 10:36:03PM +0200, Laurenz Albe wrote:
> Bossart, Nathan wrote:
> > Attached is a patch to add a pg_ls_tmpdir() function that lists the
> > contents of a specified tablespace's pgsql_tmp directory.  This is
> > very similar to the existing pg_ls_logdir() and pg_ls_waldir()
> > functions.

On Sun, Sep 30, 2018 at 10:59:20PM +0200, Christoph Moench-Tegeder wrote:
> while setting up monitoring for a new PostgreSQL instance, I noticed that
> there's no build-in way for a pg_monitor role to check the contents of
> the archive_status directory. We got pg_ls_waldir() in 10, but that
> only lists pg_wal - not it's (sic) subdirectory. 

>    It may make sense to have a generic function like
>       pg_ls_dir(dirname text, tablespace oid)
>    instead.  But maybe that's taking it too far...

I see Cristoph has another pg_ls_* function, which suggests that Laurenz idea
is good, and a generic pg_ls function is desirable.

Justin

Hi,

On 2018-09-26 22:36:03 +0200, Laurenz Albe wrote:
> 2. There already are functions "pg_ls_logdir" and "pg_ls_waldir",
>    and I can imagine new requests, e.g. pg_ls_datafiles() to list the
>    data files in a database directory.
> 
>    It may make sense to have a generic function like
> 
>       pg_ls_dir(dirname text, tablespace oid)
> 
>    instead.  But maybe that's taking it too far...

There's a generic pg_ls_dir() already - I'm now sure why a generic
function would take the tablespace oid however?

Greetings,

Andres Freund

Hi,

On 9/26/18, 3:36 PM, "Laurenz Albe" <laurenz.albe@cybertec.at> wrote:
> The patch applies, builds without warning and passes "make check-world".

Thanks for the review!

> Since pgsql_tmp is only created when the first temp file is written,
> calling the function on a newly initdb'ed data directory fails with
>
> ERROR:  could not open directory "base/pgsql_tmp": No such file or directory
>
> This should be fixed; it shouldn't be an error.

Done.

> Calling the function with a non-existing tablespace OID produces:
>
> ERROR:  could not open directory "pg_tblspc/1665/PG_12_201809121/pgsql_tmp": No such file or directory
>
> Wouldn't it be better to have a check if the tablespace exists?

Done.

> About the code:
> The catversion bump shouldn't be part of the patch, it will
> rot too quickly.  The committer is supposed to add it.

Removed.

> I think the idea to have such a function is fine, but I have two doubts:
>
> 1. Do we really need two functions, one without input argument
>    to list the default tablespace?
>    I think that anybody who uses with such a function whould
>    be able to feed the OID of "pg_default".

That seems reasonable to me.  I've removed the second version of the
function.

> 2. There already are functions "pg_ls_logdir" and "pg_ls_waldir",
>    and I can imagine new requests, e.g. pg_ls_datafiles() to list the
>    data files in a database directory.
>
>    It may make sense to have a generic function like
>
>       pg_ls_dir(dirname text, tablespace oid)
>
>    instead.  But maybe that's taking it too far...

There is an existing pg_ls_dir() function that appears to be available
only to superusers.  IMO it's also nice to break out specific "safe"
directories like this for other roles (e.g. pg_monitor).

Nathan

Re: Bossart, Nathan 2018-10-01 <69FD7E51-2B13-41FD-9438-17395C73F5BF@amazon.com>
> > 1. Do we really need two functions, one without input argument
> >    to list the default tablespace?
> >    I think that anybody who uses with such a function whould
> >    be able to feed the OID of "pg_default".
> 
> That seems reasonable to me.  I've removed the second version of the
> function.

You could make that one argument have a DEFAULT value that makes it
act on pg_default.

Christoph

Christoph Berg wrote:
> Re: Bossart, Nathan 2018-10-01 <69FD7E51-2B13-41FD-9438-17395C73F5BF@amazon.com>
> > > 1. Do we really need two functions, one without input argument
> > >     to list the default tablespace?
> > >     I think that anybody who uses with such a function whould
> > >     be able to feed the OID of "pg_default".
> > 
> > That seems reasonable to me.  I've removed the second version of the
> > function.
> 
> You could make that one argument have a DEFAULT value that makes it
> act on pg_default.

I looked at that, and I don't think you can add a DEFAULT for
system functions installed during bootstrap.
At least I failed in the attempt.

Yours,
Laurenz Albe

On 10/02/2018 08:00 AM, Laurenz Albe wrote:
> Christoph Berg wrote:
>> Re: Bossart, Nathan 2018-10-01 <69FD7E51-2B13-41FD-9438-17395C73F5BF@amazon.com>
>>>> 1. Do we really need two functions, one without input argument
>>>>      to list the default tablespace?
>>>>      I think that anybody who uses with such a function whould
>>>>      be able to feed the OID of "pg_default".
>>> That seems reasonable to me.  I've removed the second version of the
>>> function.
>> You could make that one argument have a DEFAULT value that makes it
>> act on pg_default.
> I looked at that, and I don't think you can add a DEFAULT for
> system functions installed during bootstrap.
> At least I failed in the attempt.


See the bottom of src/backend/catalog/system_views.sql starting around 
line 1010.

cheers

andrew


-- 
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 10/2/18, 7:22 AM, "Andrew Dunstan" <andrew.dunstan@2ndquadrant.com> wrote:
> On 10/02/2018 08:00 AM, Laurenz Albe wrote:
>> Christoph Berg wrote:
>>> Re: Bossart, Nathan 2018-10-01 <69FD7E51-2B13-41FD-9438-17395C73F5BF@amazon.com>
>>>>> 1. Do we really need two functions, one without input argument
>>>>>      to list the default tablespace?
>>>>>      I think that anybody who uses with such a function whould
>>>>>      be able to feed the OID of "pg_default".
>>>> That seems reasonable to me.  I've removed the second version of the
>>>> function.
>>> You could make that one argument have a DEFAULT value that makes it
>>> act on pg_default.
>> I looked at that, and I don't think you can add a DEFAULT for
>> system functions installed during bootstrap.
>> At least I failed in the attempt.
>
>
> See the bottom of src/backend/catalog/system_views.sql starting around 
> line 1010.

AFAICT the cleanest way to do this in system_views.sql is to hard-code
the pg_default tablespace OID in the DEFAULT expression.  So, it might
be best to use the two function approach if we want pg_ls_tmpdir() to
default to the pg_default tablespace.

Nathan

"Bossart, Nathan" <bossartn@amazon.com> writes:
> On 10/2/18, 7:22 AM, "Andrew Dunstan" <andrew.dunstan@2ndquadrant.com> wrote:
>> See the bottom of src/backend/catalog/system_views.sql starting around
>> line 1010.

> AFAICT the cleanest way to do this in system_views.sql is to hard-code
> the pg_default tablespace OID in the DEFAULT expression.  So, it might
> be best to use the two function approach if we want pg_ls_tmpdir() to
> default to the pg_default tablespace.

That would be pretty bletcherous, so +1 for just making two C functions.

            regards, tom lane

On 10/2/18, 11:46 AM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote:
> "Bossart, Nathan" <bossartn@amazon.com> writes:
>> On 10/2/18, 7:22 AM, "Andrew Dunstan" <andrew.dunstan@2ndquadrant.com> wrote:
>>> See the bottom of src/backend/catalog/system_views.sql starting around 
>>> line 1010.
>
>> AFAICT the cleanest way to do this in system_views.sql is to hard-code
>> the pg_default tablespace OID in the DEFAULT expression.  So, it might
>> be best to use the two function approach if we want pg_ls_tmpdir() to
>> default to the pg_default tablespace.
>
> That would be pretty bletcherous, so +1 for just making two C functions.

Alright, here's an updated patch.

Nathan

Bossart, Nathan wrote:
> >> AFAICT the cleanest way to do this in system_views.sql is to hard-code
> >> the pg_default tablespace OID in the DEFAULT expression.  So, it might
> >> be best to use the two function approach if we want pg_ls_tmpdir() to
> >> default to the pg_default tablespace.
> >
> > That would be pretty bletcherous, so +1 for just making two C functions.
> 
> Alright, here's an updated patch.

Looks, good; marking as "ready for committer".

Yours,
Laurenz Albe

On Thu, Oct 04, 2018 at 02:23:34PM +0200, Laurenz Albe wrote:
> Bossart, Nathan wrote:
>> Alright, here's an updated patch.
>
> Looks, good; marking as "ready for committer".

Like Tom, I think it is less dirty to use the two-function approach.
Committed this way with a catalog version bump.
--
Michael

On 10/4/18, 7:31 PM, "Michael Paquier" <michael@paquier.xyz> wrote:
> Committed this way with a catalog version bump.

Thanks!

Nathan