Thanks for your review.
On 1/2/22, 11:00 PM, "Amul Sul" <sulamul@gmail.com> wrote:
> On Mon, Jan 3, 2022 at 2:56 AM Andres Freund <andres@anarazel.de> wrote:
>> This generates a compiler warning:
>> https://cirrus-ci.com/task/5740581082103808?logs=mingw_cross_warning#L378
>
> Somehow, I am not getting these compiler warnings on the latest master
> head (69872d0bbe6).
I attempted to fix this by including time.h in custodian.c.
> Here are the few minor comments for the v2 version, I thought would help:
>
> + * Copyright (c) 2021, PostgreSQL Global Development Group
>
> Time to change the year :)
Fixed in v3.
> +
> + /* These operations are really just a minimal subset of
> + * AbortTransaction(). We don't have very many resources to worry
> + * about.
> + */
>
> Incorrect formatting, the first line should be empty in the multiline
> code comment.
Fixed in v3.
> + XLogRecPtr logical_rewrite_mappings_cutoff; /* can remove
> older mappings */
> + XLogRecPtr logical_rewrite_mappings_cutoff_set;
>
> Look like logical_rewrite_mappings_cutoff gets to set only once and
> never get reset, if it is true then I think that variable can be
> skipped completely and set the initial logical_rewrite_mappings_cutoff
> to InvalidXLogRecPtr, that will do the needful.
I think the problem with this is that when the cutoff is
InvalidXLogRecPtr, it is taken to mean that all logical rewrite files
can be removed. If we just used the cutoff variable, we could remove
files we need if the custodian ran before the cutoff was set. I
suppose we could initially set the cutoff to MaxXLogRecPtr to indicate
that the value is not yet set, but I see no real advantage to doing it
that way versus just using a bool. Speaking of which,
logical_rewrite_mappings_cutoff_set obviously should be a bool. I've
fixed that in v3.
Nathan