Re: Reword docs of feature "Remove temporary files after backend crash"

From: Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com>
To: Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com>
Cc: Justin Pryzby <pryzby(at)telsasoft(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Euler Taveira <euler(at)eulerto(dot)com>, Tomas Vondra <tomas(dot)vondra(at)enterprisedb(dot)com>
Subject: Re: Reword docs of feature "Remove temporary files after backend crash"
Date: 2021-10-10 13:33:32
Message-ID: CALj2ACWdDyWM=n=cLRf+KMHDa=-6n+G1qAvNMH5fQBji4FPEjA@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Sun, Oct 10, 2021 at 9:12 AM Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote:
>
> On 2021/10/10 1:25, Bharath Rupireddy wrote:
> > On Sat, Oct 9, 2021 at 9:42 PM Justin Pryzby <pryzby(at)telsasoft(dot)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.

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Peter Eisentraut 2021-10-10 14:04:31 Re: Skipping logical replication transactions on subscriber side
Previous Message Peter Eisentraut 2021-10-10 12:51:38 Re: Time to upgrade buildfarm coverage for some EOL'd OSes?