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

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)
Date: 2018-10-07 02:30:36
Message-ID: CAEepm=3Gp4499OouyDPLqwRTc0OGTnvGb102mMRq5BJMq=vGSQ@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

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.

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

Attachment Content-Type Size
0001-Relax-transactional-restrictions-on-ALTER-TYPE-.--v4.patch application/octet-stream 26.6 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
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.