On Sun, Oct 7, 2018 at 5:53 AM Tom Lane <t...@sss.pgh.pa.us> wrote: > Thomas Munro <thomas.mu...@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
0001-Relax-transactional-restrictions-on-ALTER-TYPE-.--v4.patch
Description: Binary data
