Thread: Reword docs of feature "Remove temporary files after backend crash"
Hi,
The commit [1] for the feature "Remove temporary files after backend
crash" introduced following in the docs:
+ <para>
+ When set to <literal>on</literal>, which is the default,
+ <productname>PostgreSQL</productname> will automatically remove
+ temporary files after a backend crash. If disabled, the files will be
+ retained and may be used for debugging, for example. Repeated crashes
+ may however result in accumulation of useless files.
+ </para>
The term backend means the user sessions (see from the glossary, at
[2]). It looks like the code introduced by the commit [1] i.e. the
temp table removal gets hit not only after the backend crash, but also
after checkpointer, bg writer, wal writer, auto vac launcher, logical
repl launcher and so on. It is sort of misleading to the normal users.
With the commit [3] clarifying these processes in master branch [4],
do we also need to modify the doc added by commit [1] in PG master at
least?
[1] commit cd91de0d17952b5763466cfa663e98318f26d357
Author: Tomas Vondra <tomas.vondra@postgresql.org>
Date: Thu Mar 18 16:05:03 2021 +0100
Remove temporary files after backend crash
[2] PG 14 - https://www.postgresql.org/docs/current/glossary.html#GLOSSARY-BACKEND
[3] commit d3014fff4cd4dcaf4b2764d96ad038f3be7413b0
Author: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Mon Sep 20 12:22:02 2021 -0300
Doc: add glossary term for "auxiliary process"
[4] PG master - https://www.postgresql.org/docs/devel/glossary.html
Regards,
Bharath Rupireddy.
Re: Reword docs of feature "Remove temporary files after backend crash"
From
Bharath Rupireddy
Date:
On Fri, Oct 8, 2021 at 4:27 PM Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote: > > Hi, > > The commit [1] for the feature "Remove temporary files after backend > crash" introduced following in the docs: > + <para> > + When set to <literal>on</literal>, which is the default, > + <productname>PostgreSQL</productname> will automatically remove > + temporary files after a backend crash. If disabled, the files will be > + retained and may be used for debugging, for example. Repeated crashes > + may however result in accumulation of useless files. > + </para> > > The term backend means the user sessions (see from the glossary, at > [2]). It looks like the code introduced by the commit [1] i.e. the > temp table removal gets hit not only after the backend crash, but also > after checkpointer, bg writer, wal writer, auto vac launcher, logical > repl launcher and so on. It is sort of misleading to the normal users. > With the commit [3] clarifying these processes in master branch [4], > do we also need to modify the doc added by commit [1] in PG master at > least? > > [1] commit cd91de0d17952b5763466cfa663e98318f26d357 > Author: Tomas Vondra <tomas.vondra@postgresql.org> > Date: Thu Mar 18 16:05:03 2021 +0100 > > Remove temporary files after backend crash > > [2] PG 14 - https://www.postgresql.org/docs/current/glossary.html#GLOSSARY-BACKEND > > [3] commit d3014fff4cd4dcaf4b2764d96ad038f3be7413b0 > Author: Alvaro Herrera <alvherre@alvh.no-ip.org> > Date: Mon Sep 20 12:22:02 2021 -0300 > > Doc: add glossary term for "auxiliary process" > > [4] PG master - https://www.postgresql.org/docs/devel/glossary.html Here's the patch modifying the docs slightly. Please review it. Regards, Bharath Rupireddy.
Attachment
On Sat, Oct 09, 2021 at 09:18:24PM +0530, Bharath Rupireddy wrote: > On Fri, Oct 8, 2021 at 4:27 PM Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote: > > > > Hi, > > > > The commit [1] for the feature "Remove temporary files after backend > > crash" introduced following in the docs: > > + <para> > > + When set to <literal>on</literal>, which is the default, > > + <productname>PostgreSQL</productname> will automatically remove > > + temporary files after a backend crash. If disabled, the files will be > > + retained and may be used for debugging, for example. Repeated crashes > > + may however result in accumulation of useless files. > > + </para> > > > > The term backend means the user sessions (see from the glossary, at > > [2]). It looks like the code introduced by the commit [1] i.e. the > > temp table removal gets hit not only after the backend crash, but also > > after checkpointer, bg writer, wal writer, auto vac launcher, logical > > repl launcher and so on. It is sort of misleading to the normal users. > > With the commit [3] clarifying these processes in master branch [4], > > do we also need to modify the doc added by commit [1] in PG master at > > least? > > > > [1] commit cd91de0d17952b5763466cfa663e98318f26d357 > > Author: Tomas Vondra <tomas.vondra@postgresql.org> > > Date: Thu Mar 18 16:05:03 2021 +0100 > > > > Remove temporary files after backend crash > > > > [2] PG 14 - https://www.postgresql.org/docs/current/glossary.html#GLOSSARY-BACKEND > > > > [3] commit d3014fff4cd4dcaf4b2764d96ad038f3be7413b0 > > Author: Alvaro Herrera <alvherre@alvh.no-ip.org> > > Date: Mon Sep 20 12:22:02 2021 -0300 > > > > Doc: add glossary term for "auxiliary process" > > > > [4] PG master - https://www.postgresql.org/docs/devel/glossary.html > > Here's the patch modifying the docs slightly. Please review it. I doubt there's much confusion here - crashes are treated the same. I think the fix would be to say "server crash" rather than backend crash. -- Justin
Re: Reword docs of feature "Remove temporary files after backend crash"
From
Bharath Rupireddy
Date:
On Sat, Oct 9, 2021 at 9:42 PM Justin Pryzby <pryzby@telsasoft.com> wrote: > > I doubt there's much confusion here - crashes are treated the same. I think > the fix would be to say "server crash" rather than backend crash. IIUC, the "server crash" includes any backend, auxiliary process, postmaster crash i.e. database cluster/instance crash. The commit cd91de0d1 especially added the temp file cleanup support if any backend or auxiliary process (except syslogger and stats collector) crashes. The temp file cleanup in postmaster crash does exist prior to the commit cd91de0d1. When we add the description about the new GUC introduced by the commit cd91de0d1, let's be clear as to which crash triggers the new temp file cleanup path. Regards, Bharath Rupireddy.
On 2021/10/10 1:25, Bharath Rupireddy wrote: > On Sat, Oct 9, 2021 at 9:42 PM Justin Pryzby <pryzby@telsasoft.com> wrote: >> >> I doubt there's much confusion here - crashes are treated the same. I think >> the fix would be to say "server crash" rather than backend crash. > > IIUC, the "server crash" includes any backend, auxiliary process, > postmaster crash i.e. database cluster/instance crash. The commit > cd91de0d1 especially added the temp file cleanup support if any > backend or auxiliary process (except syslogger and stats collector) Also the startup process should be in this exception list? > crashes. The temp file cleanup in postmaster crash does exist prior to > the commit cd91de0d1. > > When we add the description about the new GUC introduced by the commit > cd91de0d1, let's be clear as to which crash triggers the new temp file > cleanup path. If we really want to add this information, the description of restart_after_crash seems more proper place to do that in. remove_temp_files_after_crash works only when the processes are reinitialized because restart_after_crash is enabled. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: Reword docs of feature "Remove temporary files after backend crash"
From
Bharath Rupireddy
Date:
On Sun, Oct 10, 2021 at 9:12 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote: > > On 2021/10/10 1:25, Bharath Rupireddy wrote: > > On Sat, Oct 9, 2021 at 9:42 PM Justin Pryzby <pryzby@telsasoft.com> wrote: > >> > >> I doubt there's much confusion here - crashes are treated the same. I think > >> the fix would be to say "server crash" rather than backend crash. > > > > IIUC, the "server crash" includes any backend, auxiliary process, > > postmaster crash i.e. database cluster/instance crash. The commit > > cd91de0d1 especially added the temp file cleanup support if any > > backend or auxiliary process (except syslogger and stats collector) > > Also the startup process should be in this exception list? Yes, if the startup process fails, neither restart_after_crash nor remove_temp_files_after_crash code path is hit. > > crashes. The temp file cleanup in postmaster crash does exist prior to > > the commit cd91de0d1. > > > > When we add the description about the new GUC introduced by the commit > > cd91de0d1, let's be clear as to which crash triggers the new temp file > > cleanup path. > > If we really want to add this information, the description of > restart_after_crash seems more proper place to do that in. > remove_temp_files_after_crash works only when the processes are > reinitialized because restart_after_crash is enabled. IMO, we can add the new description as proposed in my v1 patch (after adding startup process to the exception list) to both the GUCs restart_after_crash and remove_temp_files_after_crash. And, in remove_temp_files_after_crash GUC description we can just add a note saying "this GUC is effective only when restart_after_crash is on". Also, I see that the restart_after_crash and remove_temp_files_after_crash descriptions in guc.c say "Remove temporary files after backend crash.". I think we can also modify them to "Remove temporary files after the backend or auxiliary process (except startup, syslogger and stats collector) crash. Thoughts? Regards, Bharath Rupireddy.
On 2021/10/10 22:33, Bharath Rupireddy wrote: >>> IIUC, the "server crash" includes any backend, auxiliary process, >>> postmaster crash i.e. database cluster/instance crash. The commit >>> cd91de0d1 especially added the temp file cleanup support if any >>> backend or auxiliary process (except syslogger and stats collector) We should mention not only a backend and an auxiliary processe but also background worker? Because, per Glossary, background worker is neither a backend nor an auxiliary process. Instead, maybe it's better to use "child processes" or something rather than mentioning those three processes. > IMO, we can add the new description as proposed in my v1 patch (after > adding startup process to the exception list) to both the GUCs > restart_after_crash and remove_temp_files_after_crash. And, in > remove_temp_files_after_crash GUC description we can just add a note > saying "this GUC is effective only when restart_after_crash is on". OK. > Also, I see that the restart_after_crash and > remove_temp_files_after_crash descriptions in guc.c say "Remove > temporary files after backend crash.". I think we can also modify them > to "Remove temporary files after the backend or auxiliary process > (except startup, syslogger and stats collector) crash. I'm not sure if we really need this long log message. IMO it's enough to add that information in the docs. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: Reword docs of feature "Remove temporary files after backend crash"
From
Bharath Rupireddy
Date:
On Mon, Oct 11, 2021 at 11:37 AM Fujii Masao
<masao.fujii@oss.nttdata.com> wrote:
> On 2021/10/10 22:33, Bharath Rupireddy wrote:
> >>> IIUC, the "server crash" includes any backend, auxiliary process,
> >>> postmaster crash i.e. database cluster/instance crash. The commit
> >>> cd91de0d1 especially added the temp file cleanup support if any
> >>> backend or auxiliary process (except syslogger and stats collector)
>
> We should mention not only a backend and an auxiliary processe
> but also background worker? Because, per Glossary, background worker
> is neither a backend nor an auxiliary process. Instead,
> maybe it's better to use "child processes" or something rather than
> mentioning those three processes.
If we were to use child processes (a term the glossary doesn't
define), we might end up saying postmaster child process crash, that's
not enough. We have to say things like "child process (except startup,
syslogger and stats collector) crash." IMO, let's not introduce
another term for the processes, the glossary defines many kinds of
processes already.
> > Also, I see that the restart_after_crash and
> > remove_temp_files_after_crash descriptions in guc.c say "Remove
> > temporary files after backend crash.". I think we can also modify them
> > to "Remove temporary files after the backend or auxiliary process
> > (except startup, syslogger and stats collector) crash.
>
> I'm not sure if we really need this long log message.
> IMO it's enough to add that information in the docs.
IMO let's be clear here as well for consistency reasons. I've seen
some of the long descriptions for GUCs [1]. And it seems like we don't
have any tex
So, the text for remove_temp_files_after_crash will be : "Remove
temporary files after backend or auxiliary process (except startup,
syslogger and stats collector) or background worker crash."
and for restart_after_crash: "Reinitialize server after backend crash
or auxiliary process (except startup, syslogger and stats collector)
or background worker crash."
I noticed another thing that the remove_temp_files_after_crash is
categorized as DEVELOPER_OPTIONS, shouldn't it be under
RESOURCES_DISK?
[1]
gettext_noop("Sets whether a WAL receiver should create a temporary
replication slot if no permanent slot is configured."),
gettext_noop("Writes full pages to WAL when first modified after a
checkpoint, even for a non-critical modification.")
gettext_noop("Enables backward compatibility mode for privilege checks
on large objects.")
gettext_noop("Forces a switch to the next WAL file if a "
"new file has not been started within N seconds."),
Regards,
Bharath Rupireddy.
Re: Reword docs of feature "Remove temporary files after backend crash"
From
Bharath Rupireddy
Date:
On Mon, Oct 11, 2021 at 2:53 PM Justin Pryzby <pryzby@telsasoft.com> wrote: > > On Mon, Oct 11, 2021 at 12:50:28PM +0530, Bharath Rupireddy wrote: > > I noticed another thing that the remove_temp_files_after_crash is > > categorized as DEVELOPER_OPTIONS, shouldn't it be under > > RESOURCES_DISK? > > See here: > https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=797b0fc0b078c7b4c46ef9f2d9e47aa2d98c6c63 > > The old behavior of leaving the tempfiles behind isn't expected to be useful to > uses, and the only reason to keep them was to allow debugging. > > Putting it in DEVELOPER means that it's not in the user-facing > postgresql.conf.sample. Thanks. Regards, Bharath Rupireddy.
Re: Reword docs of feature "Remove temporary files after backend crash"
From
Bharath Rupireddy
Date:
On Mon, Oct 11, 2021 at 11:37 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote: > > IMO, we can add the new description as proposed in my v1 patch (after > > adding startup process to the exception list) to both the GUCs > > restart_after_crash and remove_temp_files_after_crash. And, in > > remove_temp_files_after_crash GUC description we can just add a note > > saying "this GUC is effective only when restart_after_crash is on". > > OK. Here's a v2 patch that I could come up with. Please review it further. I've also made a CF entry - https://commitfest.postgresql.org/35/3356/ Regards, Bharath Rupireddy.
Attachment
Re: Reword docs of feature "Remove temporary files after backend crash"
From
Daniel Gustafsson
Date:
> On 12 Oct 2021, at 08:40, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote: > Here's a v2 patch that I could come up with. Please review it further. + debugging, for example. Repeated crashes may however result in + accumulation of useless files. This parameter can only be set in the I think "useless" is a bit too strong and subjective given that it's describing an unknown situation out of the ordinary. How about "outdated" or "redundant" (or something else entirely which is even better)? > I've also made a CF entry - https://commitfest.postgresql.org/35/3356/ This has been sitting the CF for quite some time, time to make a decision on it. I think it makes sense, having detailed docs around debugging is rarely a bad thing. Does anyone else have opinions? -- Daniel Gustafsson https://vmware.com/
On Fri, Apr 1, 2022 at 9:42 AM Daniel Gustafsson <daniel@yesql.se> wrote: > I think "useless" is a bit too strong and subjective given that it's describing > an unknown situation out of the ordinary. How about "outdated" or "redundant" > (or something else entirely which is even better)? It's the existing wording, though, and unrelated to the changes the patch is trying to make. It also seems accurate to me. > > I've also made a CF entry - https://commitfest.postgresql.org/35/3356/ > > This has been sitting the CF for quite some time, time to make a decision on > it. I think it makes sense, having detailed docs around debugging is rarely a > bad thing. Does anyone else have opinions? I don't like it. It seems to me that it will result in a lot of duplication in the docs, because every time we talk about something that happens in connection with a crash, we'll need to talk about this same list of exceptions. It would be reasonable to document which conditions trigger a crash-and-restart cycle and which do not in some centralized place, but not in every place where we mention crashing. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Reword docs of feature "Remove temporary files after backend crash"
From
Daniel Gustafsson
Date:
> On 1 Apr 2022, at 15:57, Robert Haas <robertmhaas@gmail.com> wrote: > On Fri, Apr 1, 2022 at 9:42 AM Daniel Gustafsson <daniel@yesql.se> wrote: >> This has been sitting the CF for quite some time, time to make a decision on >> it. I think it makes sense, having detailed docs around debugging is rarely a >> bad thing. Does anyone else have opinions? > > I don't like it. It seems to me that it will result in a lot of > duplication in the docs, because every time we talk about something > that happens in connection with a crash, we'll need to talk about this > same list of exceptions. Fair enough, that's a valid concern. Unless others object I then think we should close this patch in the CF as rejected. -- Daniel Gustafsson https://vmware.com/