Horton

Thread

Commits

  1. Add pg_ls_archive_statusdir function

  1. Function for listing archive_status directory

    Christoph Moench-Tegeder <cmt@burggraben.net> — 2018-09-30T20:59:20Z

    Hi,

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 subdirectory. It seems listing the
archive_status directory wasn't even really discussed (or my Google-Fu
betrayed me?). Of course, these days people should use streaming archiving
(but there're still environments where that's not an option); and of
course it's possible to create a wrapper function for
pg_ls_dir('pg_wal/archive_status') with SECURITY DEFINER set - but the
same could have been said about pg_ls_waldir(), and it didn't stop
anyone.

Without further ado, I present a patch to implement pg_ls_archive_status(),
which fills this gap. I believe the function name is long enough and we
don't need an extra wal in there. The patch is based on a very recent
master (just pulled and merged), but does not include the catversion
bump (avoid conflict on merge).

Is this relevant?

Regards,
Christoph

-- 
Spare Space

  2. Re: Function for listing archive_status directory

    Michael Paquier <michael@paquier.xyz> — 2018-09-30T23:21:01Z

    On Sun, Sep 30, 2018 at 10:59:20PM +0200, Christoph Moench-Tegeder wrote:
> Without further ado, I present a patch to implement pg_ls_archive_status(),
> which fills this gap. I believe the function name is long enough and we
> don't need an extra wal in there. The patch is based on a very recent
> master (just pulled and merged), but does not include the catversion
> bump (avoid conflict on merge).

Okay, could you add this patch to the next commit fest?  Here it is:
https://commitfest.postgresql.org/20/
--
Michael

  3. Re: Function for listing archive_status directory

    Christoph Moench-Tegeder <cmt@burggraben.net> — 2018-10-01T11:36:23Z

    ## Michael Paquier (michael@paquier.xyz):

> Okay, could you add this patch to the next commit fest?  Here it is:
> https://commitfest.postgresql.org/20/

And here's the patch: https://commitfest.postgresql.org/20/1813/

Regards,
Christoph

-- 
Spare Space

  4. RE: Function for listing archive_status directory

    Iwata, Aya <iwata.aya@jp.fujitsu.com> — 2018-10-04T01:03:53Z

    Hi Christoph,

I think it is convenient to be able to check the archive_status directory contents information.

I reviewed patch. It applies and passes regression test.

I checked the code. 
It refers to the patch which added pg_ls_waldir() and pg_ls_logdir(), so I think it is good.

There is one point I care about. 
All similar function are named pg_ls_***dir. It is clear these functions return directory contents information.
If the new function intends to display the contents of the directory, pg_ls_***dir style might be better (e.g. pg_ls_archive_statusdir).
But everyone know archive_status is a directory...
If you want to follow the standard naming, then you may add the dir.

Do you watch this thread?
https://www.postgresql.org/message-id/flat/92F458A2-6459-44B8-A7F2-2ADD3225046A@amazon.com
They are also discussing about generic pg_ls function.

Regards, 
Aya Iwata

  5. Re: Function for listing archive_status directory

    Christoph Moench-Tegeder <cmt@burggraben.net> — 2018-10-05T11:06:42Z

    Hi,

## Iwata, Aya (iwata.aya@jp.fujitsu.com):

> I think it is convenient to be able to check the archive_status
> directory contents information.
> 
> I reviewed patch. It applies and passes regression test.

Great, thanks!

> All similar function are named pg_ls_***dir. It is clear these functions
> return directory contents information.
> If the new function intends to display the contents of the directory,
> pg_ls_***dir style might be better (e.g. pg_ls_archive_statusdir).
> But everyone know archive_status is a directory...
> If you want to follow the standard naming, then you may add the dir.

I conciously omitted the "_dir" suffix - I'm not a great fan of long
function names, and we want to inspect the contents of archive_status
to find out about the status of the archiving process. But then, my
main concern is the functionality, not the name, we can easily change
the name. Is there any other opinion pro/contra the name?

> Do you watch this thread?
> https://www.postgresql.org/message-id/flat/92F458A2-6459-44B8-A7F2-2ADD3225046A@amazon.com
> They are also discussing about generic pg_ls function.

I'm aware of that threat, and that Michael just commited "pg_ls_tmpdir()".
I'm not that sure about Laurenz' idea regarding monitoring all the
database directories (but it doesn't hurt anybody...). Anyway, the
archive_status directory is not coupled to any specific database or
tablespace, so there's not too much overlap.

Regards,
Christoph

-- 
Spare Space

  6. RE: Function for listing archive_status directory

    Iwata, Aya <iwata.aya@jp.fujitsu.com> — 2018-10-09T00:51:34Z

    Hi Christoph,

> > All similar function are named pg_ls_***dir. It is clear these
> > functions return directory contents information.
> > If the new function intends to display the contents of the directory,
> > pg_ls_***dir style might be better (e.g. pg_ls_archive_statusdir).
> > But everyone know archive_status is a directory...
> > If you want to follow the standard naming, then you may add the dir.
> 
> I conciously omitted the "_dir" suffix - I'm not a great fan of long function
> names, and we want to inspect the contents of archive_status to find out about
> the status of the archiving process. But then, my main concern is the
> functionality, not the name, we can easily change the name. Is there any other
> opinion pro/contra the name?

I understand the reason why you have decided that name. And I agree with your opinion.

This function is useful for knowing about the status of archive log.
I didn't find any problems with the patch, so I'm marking it as "Ready for Committer".

Regards,
Aya Iwata

  7. RE: Function for listing archive_status directory

    Iwata, Aya <iwata.aya@jp.fujitsu.com> — 2018-10-09T02:14:52Z

    > I didn't find any problems with the patch, so I'm marking it as "Ready for
> Committer".

Sorry, I made a mistake. You patch currently does not apply. Kindly rebase the patch.
I'm marking it as "Waiting on Author".

Regards,
Aya Iwata

  8. Re: Function for listing archive_status directory

    Michael Paquier <michael@paquier.xyz> — 2018-10-09T02:26:42Z

    On Tue, Oct 09, 2018 at 02:14:52AM +0000, Iwata, Aya wrote:
> Sorry, I made a mistake. You patch currently does not apply. Kindly
> rebase the patch.  I'm marking it as "Waiting on Author".

Thanks Iwata-san.  I was just trying to apply the patch but it failed so
the new status is fine.  On top of taking care of the rebase, please
make sure of the following:
- Calling pg_ls_dir_files() with missing_ok set to true.
- Renaming pg_ls_archive_status to pg_ls_archive_statusdir.
We have a pretty nice consistency in the name of such functions as they
finish by *dir, so it makes lookups using for example "\df *dir" easier
to spot all the functions in the same category.

+    last modified time (mtime) of each file in the write ahead log (WAL)
+    <literal>archive_status</literal> directory. By default only superusers
Here I would mention pg_wal/archive_status.

No need for a catalog bump in what you submit on this thread, this is
taken care by committers.
--
Michael

  9. Re: Function for listing archive_status directory

    Christoph Moench-Tegeder <cmt@burggraben.net> — 2018-10-09T08:12:17Z

    ## Michael Paquier (michael@paquier.xyz):

> Thanks Iwata-san.  I was just trying to apply the patch but it failed so
> the new status is fine.  On top of taking care of the rebase, please
> make sure of the following:

OK, that was an easy one.

> - Calling pg_ls_dir_files() with missing_ok set to true.

Done.

> - Renaming pg_ls_archive_status to pg_ls_archive_statusdir.

Done.

> +    last modified time (mtime) of each file in the write ahead log (WAL)
> +    <literal>archive_status</literal> directory. By default only superusers
> Here I would mention pg_wal/archive_status.

Done.

Attached is the updated patch. I made sure the function's OID hadn't been
taken otherwise, and it compiles and works in a quick check.

Regards,
Christoph

-- 
Spare Space.

  10. Re: Function for listing archive_status directory

    Michael Paquier <michael@paquier.xyz> — 2018-10-09T13:41:52Z

    On Tue, Oct 09, 2018 at 10:12:17AM +0200, 'Christoph Moench-Tegeder' wrote:
> Attached is the updated patch. I made sure the function's OID hadn't been
> taken otherwise, and it compiles and works in a quick check.

Committed, after some slight adjustments.  Files in
pg_wal/archive_status/ have normally a size of 0 so this field does not
usually matter, but removing it complicates pg_ls_dir_files for no real
gain so I kept it.
--
Michael