Re: .ready and .done files considered harmful

From: Dipesh Pandit <dipesh(dot)pandit(at)gmail(dot)com>
To: Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>
Cc: "Bossart, Nathan" <bossartn(at)amazon(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, Jeevan Ladhe <jeevan(dot)ladhe(at)enterprisedb(dot)com>, Stephen Frost <sfrost(at)snowman(dot)net>, Andres Freund <andres(at)anarazel(dot)de>, Hannu Krosing <hannuk(at)google(dot)com>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: .ready and .done files considered harmful
Date: 2021-08-24 12:30:56
Message-ID: CAN1g5_FavEuGVETeOiLE64NpWp8XX_yW8PAsoorG6O1ryjJVZg@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Thanks for the feedback.

> > > IIUC partial WAL files are handled because the next file in the
> > > sequence with the given TimeLineID won't be there, so we will fall
> > > back to a directory scan and pick it up. Timeline history files are
> > > handled by forcing a directory scan, which should work because they
> > > always have the highest priority. Backup history files, however, do
> > > not seem to be handled. I think one approach to fixing that is to
> > > also treat backup history files similarly to timeline history files.
> > > If one is created, we force a directory scan, and the directory scan
> > > logic will consider backup history files as higher priority than
> > > everything but timeline history files.
> >
> > Backup history files are (currently) just informational and they are
> > finally processed at the end of a bulk-archiving performed by the fast
> > path. However, I feel that it is cleaner to trigger a directory scan
> > every time we add an other-than-a-regular-WAL-file, as base-backup or
> - promotion are not supposed happen so infrequently.
> + promotion are not supposed happen so frequently.

I have incorporated the changes to trigger a directory scan in case of a
backup history file. Also, updated archiver to prioritize archiving a backup
history file over regular WAL files during directory scan to make sure that
backup history file gets archived before the directory scan gets disabled
as part of archiving a regular WAL file.

> > I've been looking at the v9 patch with fresh eyes, and I still think
> > we should be able to force the directory scan as needed in
> > XLogArchiveNotify(). Unless the file to archive is a regular WAL file
> > that is > our stored location in archiver memory, we should force a
> > directory scan. I think it needs to be > instead of >= because we
> > don't know if the archiver has just completed a directory scan and
> > found a later segment to use to update the archiver state (but hasn't
> > yet updated the state in shared memory).
>
> I'm afraid that it can be seen as a violation of modularity. I feel
> that wal-emitter side should not be aware of that datail of
> archiving. Instead, I would prefer to keep directory scan as far as it
> found an smaller segment id than the next-expected segment id ever
> archived by the fast-path (if possible). This would be
> less-performant in the case out-of-order segments are frequent but I
> think the overall objective of the original patch will be kept.

Archiver selects the file with lowest segment number as part of directory
scan and the next segment number gets resets based on this file. It starts
a new sequence from here and check the availability of the next file. If
there are holes then it will continue to fall back to directory scan. This
will
continue until it finds the next sequence in order. I think this is already
handled unless I am missing something.

> Also, I think we need to make sure to set PgArch->dirScan back to true
> > at the end of pgarch_readyXlog() unless we've found a new regular WAL
> > file that we can use to reset the archiver's stored location. This
> > ensures that we'll keep doing directory scans as long as there are
> > timeline/backup history files to process.
>
> Right.

Done.

Please find the attached patch v10.

Thanks,
Dipesh

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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Daniel Gustafsson 2021-08-24 12:38:05 Re: remove internal support in pgcrypto?
Previous Message Daniel Westermann (DWE) 2021-08-24 12:04:28 Re: Tab completion for "create unlogged" a bit too lax?