Hi John,
Thanks for having a look at this.
On Wed, 8 Apr 2020 at 00:16, John Naylor <john.naylor@2ndquadrant.com> wrote:
> Overall looks good to me. Just a couple things I see:
>
> It seems _hash_log2 is still in the tree, but has no callers?
Yeah, I left it in there since it was an external function. Perhaps
we could rip it out and write something in the commit message that it
should be replaced with the newer functions. Thinking of extension
authors here.
> - max_size = 8; /* semi-arbitrary small power of 2 */
> - while (max_size < min_size + LIST_HEADER_OVERHEAD)
> - max_size *= 2;
> + max_size = pg_nextpower2_32(Max(8, min_size + LIST_HEADER_OVERHEAD));
>
> Minor nit: We might want to keep the comment that the number is
> "semi-arbitrary" here as well.
I had dropped that as the 8 part was mentioned in the comment above:
"The minimum allocation is 8 ListCell units". I can put it back, I had
just thought it was overkill.
> - 'pg_waldump', 'scripts');
> + 'pg_validatebackup', 'pg_waldump', 'scripts');
>
> This seems like a separate concern?
That's required due to the #include "lib/simplehash.h" in
pg_validatebackup.c. I have to say, I didn't really take the time to
understand all the Perl code there, but without that change, I was
getting a link error when testing on Windows, and after I added
pg_validatebackup to that array, it worked.
David