Re: .ready and .done files considered harmful

Thanks, Dipesh. The patch LGTM.

Some minor suggestions:

+ *

+ * "nextLogSegNo" identifies the next log file to be archived in a log

+ * sequence and the flag "dirScan" specifies a full directory scan to find

+ * the next log file.

IMHO, this comment should go atop of pgarch_readyXlog() as a description

of its parameters, and not in pgarch_ArchiverCopyLoop().

/*

+ * Interrupt handler for archiver

+ *

+ * There is a timeline switch and we have been notified by backend.

+ */

Instead, I would suggest having something like this:

+/*

+ * Interrupt handler for handling the timeline switch.

+ *

+ * A timeline switch has been notified, mark this event so that the next
iteration

+ * of pgarch_ArchiverCopyLoop() archives the history file, and we set the

+ * timeline to the new one for the next anticipated log segment.

+ */

Regards,

Jeevan Ladhe

On Thu, Jul 22, 2021 at 12:46 PM Dipesh Pandit <dipesh(dot)pandit(at)gmail(dot)com>
wrote:

> 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
>