Thread: Function for listing archive_status directory
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
Attachment
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
Attachment
## 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
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
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
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
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
> 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
> 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
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
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
Attachment
## 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.
Attachment
## 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.
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