Horton

Thread

Commits

  1. Relax transactional restrictions on ALTER TYPE ... ADD VALUE (redux).

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

    Thomas Munro <thomas.munro@enterprisedb.com> — 2018-10-03T01:24:17Z

    Hi Andrew, Tom, all,

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.

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

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

    Thomas Munro <thomas.munro@enterprisedb.com> — 2018-10-03T03:42:54Z

    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

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

    Thomas Munro <thomas.munro@enterprisedb.com> — 2018-10-03T04:02:34Z

    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

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

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2018-10-05T13:31:18Z

    
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

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

    Thomas Munro <thomas.munro@enterprisedb.com> — 2018-10-06T00:35:44Z

    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

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

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2018-10-06T13:00:16Z

    
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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-10-06T16:53:12Z

    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

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

    Thomas Munro <thomas.munro@enterprisedb.com> — 2018-10-07T02:30:36Z

    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

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

    Thomas Munro <thomas.munro@enterprisedb.com> — 2018-10-08T23:54:19Z

    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