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

On Wed, Oct 3, 2018 at 2:24 PM Thomas Munro
<thomas.munro@enterprisedb.com> wrote:

Over the thread for bug #14825 I posted some draft code to show one
way to save/restore the enum blacklist for parallel workers. Here's a
better version, and a new thread. 0001 is the code by Andrew Dustan
and Tom Lane that was reverted in 93a1af0b, unchanged by me except for
resolving trivial conflicts on current master. 0002 fixes the failure
seen with make installcheck when postgresql.conf says
force_parallel_mode = regress.

Added to the next commitfest.

--
Thomas Munro
http://www.enterprisedb.com

On Wed, Oct 3, 2018 at 4:42 PM Thomas Munro
<thomas.munro@enterprisedb.com> wrote:

On Wed, Oct 3, 2018 at 2:24 PM Thomas Munro
<thomas.munro@enterprisedb.com> wrote:

Over the thread for bug #14825 I posted some draft code to show one
way to save/restore the enum blacklist for parallel workers. Here's a
better version, and a new thread. 0001 is the code by Andrew Dustan
and Tom Lane that was reverted in 93a1af0b, unchanged by me except for
resolving trivial conflicts on current master. 0002 fixes the failure
seen with make installcheck when postgresql.conf says
force_parallel_mode = regress.

Added to the next commitfest.

... which promptly caused cfbot to report that the documentation
doesn't build anymore, because it used one of those old "</>" tags
that are now outlawed. Fixed.

--
Thomas Munro
http://www.enterprisedb.com

On 10/03/2018 12:02 AM, Thomas Munro wrote:

On Wed, Oct 3, 2018 at 4:42 PM Thomas Munro
<thomas.munro@enterprisedb.com> wrote:

On Wed, Oct 3, 2018 at 2:24 PM Thomas Munro
<thomas.munro@enterprisedb.com> wrote:

Over the thread for bug #14825 I posted some draft code to show one
way to save/restore the enum blacklist for parallel workers. Here's a
better version, and a new thread. 0001 is the code by Andrew Dustan
and Tom Lane that was reverted in 93a1af0b, unchanged by me except for
resolving trivial conflicts on current master. 0002 fixes the failure
seen with make installcheck when postgresql.conf says
force_parallel_mode = regress.

Added to the next commitfest.

... which promptly caused cfbot to report that the documentation
doesn't build anymore, because it used one of those old "</>" tags
that are now outlawed. Fixed.

Many thanks for doing this. Your solution seems simpler and cleaner that
what was previously proposed.

I have tested it, and confirm that without your 0002 patch there is an
error with force_parallel_mode=regress and with 0002 that error goes away.

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Sat, Oct 6, 2018 at 2:31 AM Andrew Dunstan
<andrew.dunstan@2ndquadrant.com> wrote:

On Wed, Oct 3, 2018 at 2:24 PM Thomas Munro
<thomas.munro@enterprisedb.com> wrote:

Over the thread for bug #14825 I posted some draft code to show one
way to save/restore the enum blacklist for parallel workers. Here's a
better version, and a new thread. 0001 is the code by Andrew Dustan
and Tom Lane that was reverted in 93a1af0b, unchanged by me except for
resolving trivial conflicts on current master. 0002 fixes the failure
seen with make installcheck when postgresql.conf says
force_parallel_mode = regress.

Many thanks for doing this. Your solution seems simpler and cleaner that
what was previously proposed.

I have tested it, and confirm that without your 0002 patch there is an
error with force_parallel_mode=regress and with 0002 that error goes away.

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.

--
Thomas Munro
http://www.enterprisedb.com

On 10/05/2018 08:35 PM, Thomas Munro wrote:

On Sat, Oct 6, 2018 at 2:31 AM Andrew Dunstan
<andrew.dunstan@2ndquadrant.com> wrote:

On Wed, Oct 3, 2018 at 2:24 PM Thomas Munro
<thomas.munro@enterprisedb.com> wrote:

Over the thread for bug #14825 I posted some draft code to show one
way to save/restore the enum blacklist for parallel workers. Here's a
better version, and a new thread. 0001 is the code by Andrew Dustan
and Tom Lane that was reverted in 93a1af0b, unchanged by me except for
resolving trivial conflicts on current master. 0002 fixes the failure
seen with make installcheck when postgresql.conf says
force_parallel_mode = regress.

Many thanks for doing this. Your solution seems simpler and cleaner that
what was previously proposed.

I have tested it, and confirm that without your 0002 patch there is an
error with force_parallel_mode=regress and with 0002 that error goes away.

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.

Go for it.

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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:

* 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

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

On Sun, Oct 7, 2018 at 3:30 PM Thomas Munro
<thomas.munro@enterprisedb.com> wrote:

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.

Pushed.

--
Thomas Munro
http://www.enterprisedb.com