pg_ls_tmpdir()

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