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.pan...@gmail.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
>
