On Tue, Nov 1, 2016 at 10:31 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> IMHO, your rewrite of this patch was a bit heavy-handed.
OK... Sorry for that.
> I haven't
> scrutinized the code here so maybe it was a big improvement, and if so
> fine, but if not it's better to collaborate with the author than to
> take over.
While reviewing the code, that has finished by being a large rewrite,
and that was more understandable than a review looking at all the
small tweaks and things I have been through while reading it. I have
also experimented a couple of ideas with the patch that I added, so at
the end it proves to be a gain for everybody. I think that the last
patch is an improvement, if you want to make your own opinion on the
matter looking at the differences between both patches would be the
most direct way to go.
> In any case, yeah, I think you should put that back.
Here you go with this parameter back and the allocation of the masked
buffers done beforehand, close to the moment the XLogReader is
allocated actually. I have also removed wal_consistency from
PostgresNode.pm, small buildfarm machines would really suffer on it,
and hamster is very good to track race conditions when running TAP
tests. On top of that I have replaced a bunch of 0xFFFFF thingies by
their PG_UINT_MAX equivalents to keep things cleaner.
Now, I have put back the GUC-related code exactly to the same shape as
it was originally. Here are a couple of comments regarding it after
review:
- Let's drop 'none' as a magic keyword. Users are going to use an
empty string, and the default should be defined as such IMO.
- Using an allocated array of booleans to store the values of each
RMGRs could be replaced by an integer using bitwise shifts. Your
option looks better and makes the code cleaner.
A more nitpick remark: code comments don't refer much to RMIDs, but
they use the term "resource managers" more generally. I'd suggest to
do the same.
--
Michael
