Re: thinko in basic_archive.c

On Fri, Oct 21, 2022 at 10:43 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
>
> Thanks, but we don't need to wipe out the all bytes. Just putting \0
> at the beginning of the buffer is sufficient.

Nah, that's not a clean way IMO.

> And the Memset() at the
> beginning of basic_archive_file_internal is not needed since that
> static variables are initially initialized to zeros.

Removed. MemSet() after durable_rename() would be sufficient.

> This is not necessarily needed, but it might be better we empty
> tempfilepath after unlinking the file.

I think it's not necessary as the archiver itself is shutting down and
I don't think the server calls the shutdown callback twice. However,
if we want basic_archive_shutdown() to be more protective against
multiple calls (for any reason that we're missing), we can have a
static local variable to quickly exit if the callback is already
called. instead of MemSet(), but that's not needed I guess.

> +      expectation that a value will soon be provided. Care must be taken when
> +      multiple servers are archiving to the same
> +      <varname>basic_archive.archive_library</varname> directory as they all
> +      might try to archive the same WAL file.
>
> I don't understand what kind of care should be taken by reading this..

It's just a notice, however I agree with you that it may be confusing.
I've removed it.

Please review the attached v4 patch.

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com