Re: .ready and .done files considered harmful

From: Dipesh Pandit <dipesh(dot)pandit(at)gmail(dot)com>
To: Robert Haas <robertmhaas(at)gmail(dot)com>
Cc: Stephen Frost <sfrost(at)snowman(dot)net>, Andres Freund <andres(at)anarazel(dot)de>, Hannu Krosing <hannuk(at)google(dot)com>, Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: .ready and .done files considered harmful
Date: 2021-07-22 07:16:07
Message-ID: CAN1g5_FcxZYg3=uC0ScpoJzxPaO087FEqgr+KXdxy7RK_+Y-Kw@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi,

> some comments on v2.
Thanks for your comments. I have incorporated the changes
and updated a new patch. Please find the details below.

> On the timeline switch, setting a flag should be enough, I don't think
> that we need to wake up the archiver. Because it will just waste the
> scan cycle.
Yes, I modified it.

> Why do we need multi level interfaces? I mean instead of calling first
> XLogArchiveNotifyTLISwitch and then calling PgArchNotifyTLISwitch,
> can't we directly call PgArchNotifyTLISwitch()?
Yes, multilevel interfaces are not required. Removed extra interface.

> + if (timeline_switch)
> + {
> + /* Perform a full directory scan in next cycle */
> + dirScan = true;
> + timeline_switch = false;
> + }

> I suggest you can add some comments atop this check.
Added comment to specify the action required in case of a
timeline switch.

> I think you should use %m in the error message so that it also prints
> the OS error code.
Done.

> Why is this a global variable? I mean whenever you enter the function
> pgarch_ArchiverCopyLoop(), this can be set to true and after that you
> can pass this as inout parameter to pgarch_readyXlog() there in it can
> be conditionally set to false once we get some segment and whenever
> the timeline switch we can set it back to the true.
Yes, It is not necessary to have global scope for "dirScan". Changed
the scope to local for "dirScan" and "nextLogSegNo".

PFA patch v3.

Thanks,
Dipesh

Attachment Content-Type Size
v3-0001-mitigate-directory-scan-for-WAL-archiver.patch text/x-patch 9.5 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Thomas Munro 2021-07-22 07:30:04 Re: A qsort template
Previous Message Julien Rouhaud 2021-07-22 07:04:19 Re: Hook for extensible parsing.