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

At Mon, 5 Dec 2022 12:06:11 +0530, Sravan Kumar <sravanvcybage(at)gmail(dot)com> wrote in
> timeout = PGARCH_AUTOWAKE_INTERVAL - (curtime - last_copy_time);
> It so happens that last_copy_time and curtime are always set at the same
> time which always makes timeout equal (actually roughly equal) to
> PGARCH_AUTOWAKE_INTERVAL.

Oooo *^^*.

> This behaviour was different before the commit:
> d75288fb27b8fe0a926aaab7d75816f091ecdc27,
> in which the archiver keeps track of how much time has elapsed since
> last_copy_time
> in case there was a signal, and it results in a smaller subsequent value of
> timeout, until timeout is zero. This also avoids calling
> pgarch_ArchiverCopyLoop
> before PGARCH_AUTOWAKE_INTERVAL in case there's an intermittent signal.

Yes, WaitLatch() (I believe) no longer makes a spurious wakeup.

> With the current changes it may be okay to always sleep for
> PGARCH_AUTOWAKE_INTERVAL,
> but that means curtime and last_copy_time are no more needed.

I think you're right.

> I would like to validate if my understanding is correct, and which of the
> behaviours we would like to retain.

As my understanding the patch didn't change the copying behavior of
the function. I think we should simplify the loop by removing
last_copy_time and curtime in the "if (!time_to_stop)" block. Then we
can remove the variable "timeout" and the "if (timeout > 0)"
branch. Are you willing to work on this?

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center