Re: Question regarding "Make archiver process an auxiliary process. commit"

From: Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>
To: sravanvcybage(at)gmail(dot)com
Cc: bharath(dot)rupireddyforpostgres(at)gmail(dot)com, pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Re: Question regarding "Make archiver process an auxiliary process. commit"
Date: 2022-12-07 06:19:39
Message-ID: 20221207.151939.662411275254345514.horikyota.ntt@gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

At Wed, 7 Dec 2022 11:28:23 +0530, Sravan Kumar <sravanvcybage(at)gmail(dot)com> wrote in
> On Tue, Dec 6, 2022 at 5:24 PM Bharath Rupireddy <
> bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>
> > On Tue, Dec 6, 2022 at 4:57 PM Sravan Kumar <sravanvcybage(at)gmail(dot)com>
> > wrote:
> > >
> > > Thank you for the feedback.
> > >
> > > I'll be glad to help with the fix. Here's the patch for review.
> >
> > Thanks. +1 for fixing this.
> >> I would like to quote recent discussions on reducing the useless
> >> wakeups or increasing the sleep/hibernation times in various processes
> >> to reduce the power savings [1] [2] [3] [4] [5]. With that in context,
> >> does the archiver need to wake up every 60 sec at all when its latch
> >> gets set (PgArchWakeup()) whenever the server switches to a new WAL
> >> file? What happens if we get rid of PGARCH_AUTOWAKE_INTERVAL and rely
> >> on its latch being set? If required, we can spread PgArchWakeup() to
> >> more places, no?
> >
> >
> I like the idea of not having to wake up intermittently and probably we
> should start a discussion about it.
>
> I see the following comment in PgArchWakeup().
>
> /*
> * We don't acquire ProcArrayLock here. It's actually fine because
> * procLatch isn't ever freed, so we just can potentially set the wrong
> * process' (or no process') latch. Even in that case the archiver will
> * be relaunched shortly and will start archiving.
> */
>
> While I do not fully understand the comment, it gives me an impression that
> the SetLatch() done here is counting on the timeout to wake up the archiver
> in some cases where the latch is wrongly set.

It is telling about the first iteration of archive process, not
periodical wakeups. So I think it is doable by considering how to
handle incomplete archiving iterations.

> The proposed idea is a behaviour change while this thread intends to clean
> up some code that's
> a result of the mentioned commit. So probably the proposed idea can be
> discussed as a separate thread.

Agreed.

--
Kyotaro Horiguchi
NTT Open Source Software Center

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Amit Langote 2022-12-07 07:01:18 Re: ExecRTCheckPerms() and many prunable partitions
Previous Message Kyotaro Horiguchi 2022-12-07 06:01:02 Re: Question regarding "Make archiver process an auxiliary process. commit"