On Tue, Apr 7, 2020 at 8:26 PM David Rowley <dgrowleyml@gmail.com> wrote:
>
> 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.
I'm not the best judge of where to draw the line for extensions, but
this function does have a name beginning with an underscore, which to
me is a red flag that it's internal in nature.
> > 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.
Oh I see now, nevermind.
> > - '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.
Hmm. Does pg_bitutils.h need something like this?
#ifndef FRONTEND
extern PGDLLIMPORT const uint8 pg_leftmost_one_pos[256];
extern PGDLLIMPORT const uint8 pg_rightmost_one_pos[256];
extern PGDLLIMPORT const uint8 pg_number_of_ones[256];
#else
extern const uint8 pg_leftmost_one_pos[256];
extern const uint8 pg_rightmost_one_pos[256];
extern const uint8 pg_number_of_ones[256];
#endif
--
John Naylor https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
