Re: .ready and .done files considered harmful

Поиск
Список
Период
Сортировка
От Jeevan Ladhe
Тема Re: .ready and .done files considered harmful
Дата
Msg-id CAOgcT0Mggk4KZ8Lz-icYpJWY3c9WC3zgGCxXE_NyNgmJWNPYgQ@mail.gmail.com
обсуждение исходный текст
Ответ на Re: .ready and .done files considered harmful  (Dipesh Pandit <dipesh.pandit@gmail.com>)
Ответы Re: .ready and .done files considered harmful
Список pgsql-hackers
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.pandit@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

В списке pgsql-hackers по дате отправления:

Предыдущее
От: Erik Rijkers
Дата:
Сообщение: Re: SQL/JSON: JSON_TABLE
Следующее
От: Peter Smith
Дата:
Сообщение: Re: logical replication empty transactions