On Tue, Apr 7, 2020 at 8:26 PM David Rowley <dgrowle...@gmail.com> wrote: > > Hi John, > > Thanks for having a look at this. > > On Wed, 8 Apr 2020 at 00:16, John Naylor <john.nay...@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
