Thread: Relax transactional restrictions on ALTER ENUM ... ADD TYPE (redux)
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
Attachment
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
Attachment
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
Attachment
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
Attachment
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
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