On Sun, Oct 7, 2018 at 5:53 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Thomas Munro <thomas.munro@enterprisedb.com> writes:
> > Thanks. Here is a version squashed into one commit, with a decent
> > commit message and a small improvement: the code to create the hash
> > table is moved into a static function, to avoid repetition. I will
> > push this to master early next week, unless there is more feedback or
> > one of you would prefer to do that.
>
> Nitpicky gripes:
Thanks for the review.
> * In the commit message's references to prior commits, I think it
> should be standard to refer to master-branch commit hashes unless
> there's some really good reason to do otherwise (and then you should
> say that this commit is on branch X). Your reference to the revert
> commit is pointing to the REL_10_STABLE back-patch commit.
Oops, fixed.
> * In the (de)serialization code, it seems kinda ugly to me to use "Oid"
> as the type of the initial count-holding value, rather than "int".
> I suppose you did that to avoid alignment issues in case Oid should
> someday be a different size than "int", but it would be a good thing
> to have a comment explaining that and pointing out specifically that
> the first item is a count not an OID. (Right now, a reader has to
> reverse-engineer that fact, which I do not think is polite to the
> reader.)
I got rid of the leading size and used InvalidOid as a terminator
instead, with comments to make that clear. The alternative would be a
struct with a size and then a flexible array member, but there seems
to be no real advantage to that.
> * Also, I don't much like the lack of any cross-check that
> SerializeEnumBlacklist has been passed the correct amount of space.
> I think there should be at least an assert at the end that it hasn't
> overrun the space the caller gave it. As-is, there's exactly no
> protection against the possibility that the hash traversal produced a
> different number of entries than what EstimateEnumBlacklistSpace saw.
I added a couple of assertions.
--
Thomas Munro
http://www.enterprisedb.com
