|From:||Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com>|
|To:||Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>|
|Cc:||Andrew Dunstan <andrew(dot)dunstan(at)2ndquadrant(dot)com>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org>|
|Subject:||Re: Relax transactional restrictions on ALTER ENUM ... ADD TYPE (redux)|
|Views:||Raw Message | Whole Thread | Download mbox|
On Sun, Oct 7, 2018 at 5:53 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> 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:
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.
> * 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
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.
|Next Message||Amit Kapila||2018-10-07 04:58:25||Re: now() vs transaction_timestamp()|
|Previous Message||Alvaro Herrera||2018-10-07 01:15:32||Re: pg_upgrade failed with ERROR: null relpartbound for relation 18159 error.|