Re: Relax transactional restrictions on ALTER ENUM ... ADD TYPE (redux)

Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)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:

* 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.

* 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.)

* 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.

regards, tom lane