Thread: WIP: extensible enums
Attached is a WIP patch that allows enums to be extended with additional
labels arbitrarily. As previously discussed, it works by adding an
explicit sort order column to pg_enum. It keeps track of whether the
labels are correctly sorted by oid value, and if so uses that for
comparison, so the possible performance impact on existing uses, and on
almost all cases where a label is added at the end of the list, should
be negligible.
Open items include
* some additional error checking required
* missing documentation
* pg_upgrade considerations
To add a label at the end of the list, do:
ALTER TYPE myenum ADD 'newlabel';
To add a label somewhere else, do:
ALTER TYPE myenum ADD 'newlabel' BEFORE 'existinglabel';
or
ALTER TYPE myenum ADD 'newlabel' AFTER 'existinglabel';
I'm not wedded to the syntax. Let the bikeshedding begin.
cheers
andrew
Attachment
Excerpts from Andrew Dunstan's message of lun ago 23 05:35:09 -0400 2010: > To add a label at the end of the list, do: > > ALTER TYPE myenum ADD 'newlabel'; > > To add a label somewhere else, do: > > ALTER TYPE myenum ADD 'newlabel' BEFORE 'existinglabel'; > > or > > ALTER TYPE myenum ADD 'newlabel' AFTER 'existinglabel'; What do you need AFTER for? Seems to me that BEFORE should be enough. (You already have the unadorned syntax for adding an item after the last one, which is the corner case that BEFORE alone doesn't cover). -- Álvaro Herrera <alvherre@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
On Aug 23, 2010, at 2:35 AM, Andrew Dunstan wrote: > I'm not wedded to the syntax. Let the bikeshedding begin. Seems pretty good to me as-is. David
On Mon, August 23, 2010 11:49 am, Alvaro Herrera wrote: > Excerpts from Andrew Dunstan's message of lun ago 23 05:35:09 -0400 2010: > >> To add a label at the end of the list, do: >> >> ALTER TYPE myenum ADD 'newlabel'; >> >> To add a label somewhere else, do: >> >> ALTER TYPE myenum ADD 'newlabel' BEFORE 'existinglabel'; >> >> or >> >> ALTER TYPE myenum ADD 'newlabel' AFTER 'existinglabel'; > > What do you need AFTER for? Seems to me that BEFORE should be enough. > (You already have the unadorned syntax for adding an item after the last > one, which is the corner case that BEFORE alone doesn't cover). > You're right. Strictly speaking we don't need it. But it doesn't hurt much to provide it for a degree of symmetry. cheers andrew
On Mon, August 23, 2010 11:49 am, Alvaro Herrera wrote: > Excerpts from Andrew Dunstan's message of lun ago 23 05:35:09 -0400 2010: > >> To add a label at the end of the list, do: >> >> ALTER TYPE myenum ADD 'newlabel'; >> >> To add a label somewhere else, do: >> >> ALTER TYPE myenum ADD 'newlabel' BEFORE 'existinglabel'; >> >> or >> >> ALTER TYPE myenum ADD 'newlabel' AFTER 'existinglabel'; > > What do you need AFTER for? Seems to me that BEFORE should be enough. > (You already have the unadorned syntax for adding an item after the last > one, which is the corner case that BEFORE alone doesn't cover). > You're right. Strictly speaking we don't need it. But it doesn't hurt much to provide it for a degree of symmetry. cheers andrew
"Andrew Dunstan" <andrew@dunslane.net> writes:
> On Mon, August 23, 2010 11:49 am, Alvaro Herrera wrote:
>> What do you need AFTER for? Seems to me that BEFORE should be enough.
>> (You already have the unadorned syntax for adding an item after the last
>> one, which is the corner case that BEFORE alone doesn't cover).
> You're right. Strictly speaking we don't need it. But it doesn't hurt much
> to provide it for a degree of symmetry.
I'm with Alvaro: drop the AFTER variant. It provides more than one way
to do the same thing, which isn't that exciting, and it's also going to
make it harder to document the performance issues. Without that, you
can just say "ADD BEFORE will make the enum slower, but plain ADD won't"
(ignoring the issue of OID wraparound, which'll confuse matters in any
case).
regards, tom lane
> You're right. Strictly speaking we don't need it. But it doesn't hurt much
> to provide it for a degree of symmetry.
Swami Josh predicts that if we don't add AFTER now, we'll be adding it
in 2 years when enough people complain about it.
-- -- Josh Berkus PostgreSQL Experts Inc.
http://www.pgexperts.com
On 23 August 2010 10:35, Andrew Dunstan <andrew@dunslane.net> wrote: > > Attached is a WIP patch that allows enums to be extended with additional > labels arbitrarily. As previously discussed, it works by adding an explicit > sort order column to pg_enum. It keeps track of whether the labels are > correctly sorted by oid value, and if so uses that for comparison, so the > possible performance impact on existing uses, and on almost all cases where > a label is added at the end of the list, should be negligible. > > Open items include > > * some additional error checking required > * missing documentation > * pg_upgrade considerations > > > To add a label at the end of the list, do: > > ALTER TYPE myenum ADD 'newlabel'; > > To add a label somewhere else, do: > > ALTER TYPE myenum ADD 'newlabel' BEFORE 'existinglabel'; > > or > > ALTER TYPE myenum ADD 'newlabel' AFTER 'existinglabel'; > > > I'm not wedded to the syntax. Let the bikeshedding begin. > > cheers > > andrew When you write the supporting doc changes, you might want to add a note in to mention that you cannot remove a label once it has been added. Will the modified enums remain intact after a dump/restore? -- Thom Brown Registered Linux user: #516935
On Mon, Aug 23, 2010 at 11:49:41AM -0400, Alvaro Herrera wrote: > Excerpts from Andrew Dunstan's message of lun ago 23 05:35:09 -0400 2010: > > > To add a label at the end of the list, do: > > > > ALTER TYPE myenum ADD 'newlabel'; > > > > To add a label somewhere else, do: > > > > ALTER TYPE myenum ADD 'newlabel' BEFORE 'existinglabel'; > > > > or > > > > ALTER TYPE myenum ADD 'newlabel' AFTER 'existinglabel'; > > What do you need AFTER for? Seems to me that BEFORE should be enough. > (You already have the unadorned syntax for adding an item after the last > one, which is the corner case that BEFORE alone doesn't cover). Making things easier for the users is a good thing all by itself :) +1 for including both BEFORE and AFTER. Would it be worth it to allow something like FIRST and LAST? ALTER TYPE myenum ADD 'newlabel' FIRST; ALTER TYPE myenum ADD 'newlabel' LAST; Cheers, David. -- David Fetter <david@fetter.org> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fetter@gmail.com iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
On Mon, Aug 23, 2010 at 01:54:40PM -0400, Tom Lane wrote: > "Andrew Dunstan" <andrew@dunslane.net> writes: > > On Mon, August 23, 2010 11:49 am, Alvaro Herrera wrote: > >> What do you need AFTER for? Seems to me that BEFORE should be > >> enough. (You already have the unadorned syntax for adding an > >> item after the last one, which is the corner case that BEFORE > >> alone doesn't cover). > > > You're right. Strictly speaking we don't need it. But it doesn't > > hurt much to provide it for a degree of symmetry. > > I'm with Alvaro: drop the AFTER variant. It provides more than one > way to do the same thing, which isn't that exciting, Not to you, maybe, but to users, it's really handy to have intuitive, rather than strictly orthogonal, ways to do things. Cheers, David. -- David Fetter <david@fetter.org> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fetter@gmail.com iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
Josh Berkus <josh@agliodbs.com> writes:
> Swami Josh predicts that if we don't add AFTER now, we'll be adding it
> in 2 years when enough people complain about it.
If it's not there, no one will ever miss it. You might as well argue
that there should be a way of creating a foreign key reference by
ALTER'ing the referenced table instead of the referencing table.
Sure, if the SQL committee was into symmetry, they might have provided
such a thing. But they didn't and no one misses it.
regards, tom lane
On Mon, Aug 23, 2010 at 1:54 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> "Andrew Dunstan" <andrew@dunslane.net> writes:
>> On Mon, August 23, 2010 11:49 am, Alvaro Herrera wrote:
>>> What do you need AFTER for? Seems to me that BEFORE should be enough.
>>> (You already have the unadorned syntax for adding an item after the last
>>> one, which is the corner case that BEFORE alone doesn't cover).
>
>> You're right. Strictly speaking we don't need it. But it doesn't hurt much
>> to provide it for a degree of symmetry.
>
> I'm with Alvaro: drop the AFTER variant. It provides more than one way
> to do the same thing, which isn't that exciting, and it's also going to
> make it harder to document the performance issues. Without that, you
> can just say "ADD BEFORE will make the enum slower, but plain ADD won't"
> (ignoring the issue of OID wraparound, which'll confuse matters in any
> case).
But what if you want to insert an OID at the end? You can't do it if
all you've got is BEFORE:
CREATE TYPE colors AS ENUM ('red', 'green', 'blue');
If I want it to become ('red', 'green', 'blue', 'orange'), what am I to do?
On 23 August 2010 19:25, Joseph Adams <joeyadams3.14159@gmail.com> wrote:
> On Mon, Aug 23, 2010 at 1:54 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> "Andrew Dunstan" <andrew@dunslane.net> writes:
>>> On Mon, August 23, 2010 11:49 am, Alvaro Herrera wrote:
>>>> What do you need AFTER for? Seems to me that BEFORE should be enough.
>>>> (You already have the unadorned syntax for adding an item after the last
>>>> one, which is the corner case that BEFORE alone doesn't cover).
>>
>>> You're right. Strictly speaking we don't need it. But it doesn't hurt much
>>> to provide it for a degree of symmetry.
>>
>> I'm with Alvaro: drop the AFTER variant. It provides more than one way
>> to do the same thing, which isn't that exciting, and it's also going to
>> make it harder to document the performance issues. Without that, you
>> can just say "ADD BEFORE will make the enum slower, but plain ADD won't"
>> (ignoring the issue of OID wraparound, which'll confuse matters in any
>> case).
>
> But what if you want to insert an OID at the end? You can't do it if
> all you've got is BEFORE:
>
> CREATE TYPE colors AS ENUM ('red', 'green', 'blue');
>
> If I want it to become ('red', 'green', 'blue', 'orange'), what am I to do?
>
ALTER TYPE colors ADD 'orange';
--
Thom Brown
Registered Linux user: #516935
> If it's not there, no one will ever miss it. You might as well argue
> that there should be a way of creating a foreign key reference by
> ALTER'ing the referenced table instead of the referencing table.
> Sure, if the SQL committee was into symmetry, they might have provided
> such a thing. But they didn't and no one misses it.
That's a very different situation, since the relationship is not
symmetrical, and it would take far more than a single keyword. Analogy
fail.
And one of the reasons people don't miss it is because far too many
users don't use FKs in the first place. ;-( The only reason why users
wouldn't notice the absence of AFTER (or, more likely, try it and then
ask on IRC for error message diagnosis) is because they're not using the
feature. (In which case it doesn't matter how it operates)
Docs which say "Add new enums BEFORE the enum you want to add them to,
and if you need to add an enum at the end, then add it without the
BEFORE keyword" is unnecessarily confusing to users. Saying "Add new
enum values using the BEFORE or AFTER keyword before or after the
appropriate value" is vastly easier to understand.
I really don't see the value in making a command substantially less
intuitive in order to avoid a single keyword, unless it affects areas of
Postgres outside of this particular command.
-- -- Josh Berkus PostgreSQL Experts Inc.
http://www.pgexperts.com
Thom Brown <thom@linux.com> writes:
> On 23 August 2010 19:25, Joseph Adams <joeyadams3.14159@gmail.com> wrote:
>> But what if you want to insert an OID at the end?
> ALTER TYPE colors ADD 'orange';
Alternatively, if people are dead set on symmetry, what we should do
to simplify is drop *this* syntax, and just have the BEFORE and AFTER
variants.
regards, tom lane
On 23/08/10 22:06, Tom Lane wrote: > Thom Brown<thom@linux.com> writes: >> On 23 August 2010 19:25, Joseph Adams<joeyadams3.14159@gmail.com> wrote: >>> But what if you want to insert an OID at the end? > >> ALTER TYPE colors ADD 'orange'; > > Alternatively, if people are dead set on symmetry, what we should do > to simplify is drop *this* syntax, and just have the BEFORE and AFTER > variants. Then you need to know the last existing value to add a new one to the end. Seems awkward. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
Josh Berkus <josh@agliodbs.com> writes:
> I really don't see the value in making a command substantially less
> intuitive in order to avoid a single keyword, unless it affects areas of
> Postgres outside of this particular command.
It's the three variants to do two things that I find unintuitive.
As I mentioned a minute ago, dropping the "abbreviated" syntax and
just having BEFORE and AFTER would be a good way of achieving
symmetry if you find that important.
regards, tom lane
On Mon, Aug 23, 2010 at 3:06 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Thom Brown <thom@linux.com> writes: >> On 23 August 2010 19:25, Joseph Adams <joeyadams3.14159@gmail.com> wrote: >>> But what if you want to insert an OID at the end? > >> ALTER TYPE colors ADD 'orange'; > > Alternatively, if people are dead set on symmetry, what we should do > to simplify is drop *this* syntax, and just have the BEFORE and AFTER > variants. FWIW, I think Andrew's originally proposed syntax is fine and useful, and we should just go with it. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company
On Mon, August 23, 2010 3:20 pm, Heikki Linnakangas wrote: > On 23/08/10 22:06, Tom Lane wrote: >> Thom Brown<thom@linux.com> writes: >>> On 23 August 2010 19:25, Joseph Adams<joeyadams3.14159@gmail.com> >>> wrote: >>>> But what if you want to insert an OID at the end? >> >>> ALTER TYPE colors ADD 'orange'; >> >> Alternatively, if people are dead set on symmetry, what we should do >> to simplify is drop *this* syntax, and just have the BEFORE and AFTER >> variants. > > Then you need to know the last existing value to add a new one to the > end. Seems awkward. > I agree. This is a non-starter, I think. The most common case in my experience is where the user doesn't care at all about the order, and just wants to add a new label. We should make that as easy as possible, especially since it's the most efficient. cheers andrew
On Aug 23, 2010, at 12:20 PM, Tom Lane wrote: > Josh Berkus <josh@agliodbs.com> writes: >> I really don't see the value in making a command substantially less >> intuitive in order to avoid a single keyword, unless it affects areas of >> Postgres outside of this particular command. > > It's the three variants to do two things that I find unintuitive. I strongly suspect that you are in the minority on this one. Best, David
On 8/23/10 12:20 PM, Tom Lane wrote:
> Josh Berkus <josh@agliodbs.com> writes:
>> I really don't see the value in making a command substantially less
>> intuitive in order to avoid a single keyword, unless it affects areas of
>> Postgres outside of this particular command.
>
> It's the three variants to do two things that I find unintuitive.
Actually, it's 3 different things:
1. BEFORE adds a value before the value cited.
2. AFTER adds a value after the value cited.
3. unqualified adds a value at the end.
The fact that AFTER allows you to add a value at the end is
circumstantial overlap. While executing an AFTER, you wouldn't *know*
that you were adding it to the end, necessarily.
The other reason to have AFTER is that, in scripts, the user may not
have the before value handy due to context (i.e. dynamically building an
enum).
Anyway, this'll still be useful with BEFORE only. I'm just convinced
that we'll end up adding AFTER in 9.2 or 9.3 after we get a bunch of
user complaints and questions. So why not add it now?
-- -- Josh Berkus PostgreSQL Experts Inc.
http://www.pgexperts.com
Josh Berkus wrote: > On 8/23/10 12:20 PM, Tom Lane wrote: > > Josh Berkus <josh@agliodbs.com> writes: > >> I really don't see the value in making a command substantially less > >> intuitive in order to avoid a single keyword, unless it affects areas of > >> Postgres outside of this particular command. > > > > It's the three variants to do two things that I find unintuitive. > > Actually, it's 3 different things: > > 1. BEFORE adds a value before the value cited. > 2. AFTER adds a value after the value cited. > 3. unqualified adds a value at the end. > > The fact that AFTER allows you to add a value at the end is > circumstantial overlap. While executing an AFTER, you wouldn't *know* > that you were adding it to the end, necessarily. > > The other reason to have AFTER is that, in scripts, the user may not > have the before value handy due to context (i.e. dynamically building an > enum). > > Anyway, this'll still be useful with BEFORE only. I'm just convinced > that we'll end up adding AFTER in 9.2 or 9.3 after we get a bunch of > user complaints and questions. So why not add it now? CREATE ENUM in PG 9.0 allows you to create an enum with no columns, e.g.: test=> CREATE TYPE etest AS ENUM ();CREATE TYPE so I think we have to have the ability add an enum without a before/after. This ability was added for pg_upgrade. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
Andrew Dunstan wrote: > > Attached is a WIP patch that allows enums to be extended with additional > labels arbitrarily. As previously discussed, it works by adding an > explicit sort order column to pg_enum. It keeps track of whether the > labels are correctly sorted by oid value, and if so uses that for > comparison, so the possible performance impact on existing uses, and on > almost all cases where a label is added at the end of the list, should > be negligible. > > Open items include > > * some additional error checking required > * missing documentation > * pg_upgrade considerations I looked at the pg_upgrade ramifications of this change and it seems some adjustments will have to be made. Right now pg_upgrade creates an empty enum type: CREATE TYPE etest AS ENUM (); and then it calls EnumValuesCreate() to create the enum labels. EnumValuesCreate() is called from within DefineEnum() where the enum type is created, and that assumes the enums are always created initially sorted. That would not be true when pg_upgrade is calling EnumValuesCreate() directly with oid assignment as part of an upgrade. I think the cleanest solution would be to modify pg_dump.c so that it creates an empty ENUM type like before, but uses the new ALTER TYPE myenum ADD 'newlabel' syntax to add the enum labels (with oid assignment like we do for CREATE TYPE, etc.) The existing code had to hack to call EnumValuesCreate() but with this new syntax it will no longer be necessary. The call to EnumValuesCreate() for enums is the only time pg_upgrade_support calls into a function rather than just assigning an oid to a global variable, so it would be great to remove that last direct function call usage. Does this code handle the case where CREATE ENUM oid wraps around while the enum label oids are being assigned? Does our existing code handle that case? I also noticed you grab an oid for the new type using the oid counter without trying to make it in sorted order. Seems that would be possible for adding enums to the end of the list, and might not be worth it. A quick hack might be to just try of an oid+1 from the last enum and see if that causes a conflict with the pg_enum.oid index. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
"David E. Wheeler" <david@kineticode.com> writes:
> On Aug 23, 2010, at 12:20 PM, Tom Lane wrote:
>> It's the three variants to do two things that I find unintuitive.
> I strongly suspect that you are in the minority on this one.
Yeah, seems like I'm losing the argument. Oh well.
regards, tom lane
Bruce Momjian <bruce@momjian.us> writes:
> I also noticed you grab an oid for the new type using the oid counter
> without trying to make it in sorted order. Seems that would be possible
> for adding enums to the end of the list, and might not be worth it. A
> quick hack might be to just try of an oid+1 from the last enum and see
> if that causes a conflict with the pg_enum.oid index.
That wouldn't be race-condition-safe.
regards, tom lane
On 08/23/2010 07:12 PM, Bruce Momjian wrote: > Josh Berkus wrote: >> On 8/23/10 12:20 PM, Tom Lane wrote: >>> Josh Berkus<josh@agliodbs.com> writes: >>>> I really don't see the value in making a command substantially less >>>> intuitive in order to avoid a single keyword, unless it affects areas of >>>> Postgres outside of this particular command. >>> It's the three variants to do two things that I find unintuitive. >> Actually, it's 3 different things: >> >> 1. BEFORE adds a value before the value cited. >> 2. AFTER adds a value after the value cited. >> 3. unqualified adds a value at the end. >> >> The fact that AFTER allows you to add a value at the end is >> circumstantial overlap. While executing an AFTER, you wouldn't *know* >> that you were adding it to the end, necessarily. >> >> The other reason to have AFTER is that, in scripts, the user may not >> have the before value handy due to context (i.e. dynamically building an >> enum). >> >> Anyway, this'll still be useful with BEFORE only. I'm just convinced >> that we'll end up adding AFTER in 9.2 or 9.3 after we get a bunch of >> user complaints and questions. So why not add it now? > CREATE ENUM in PG 9.0 allows you to create an enum with no columns, > e.g.: > > test=> CREATE TYPE etest AS ENUM (); > CREATE TYPE > > so I think we have to have the ability add an enum without a > before/after. This ability was added for pg_upgrade. > No we don't. pg_upgrade calls a C function. There is no support for this at the SQL level AIUI. And the ability to add labels at arbitrary positions in the sort order is an essential part of this feature. cheers andrew
Andrew Dunstan wrote: > > > On 08/23/2010 07:12 PM, Bruce Momjian wrote: > > Josh Berkus wrote: > >> On 8/23/10 12:20 PM, Tom Lane wrote: > >>> Josh Berkus<josh@agliodbs.com> writes: > >>>> I really don't see the value in making a command substantially less > >>>> intuitive in order to avoid a single keyword, unless it affects areas of > >>>> Postgres outside of this particular command. > >>> It's the three variants to do two things that I find unintuitive. > >> Actually, it's 3 different things: > >> > >> 1. BEFORE adds a value before the value cited. > >> 2. AFTER adds a value after the value cited. > >> 3. unqualified adds a value at the end. > >> > >> The fact that AFTER allows you to add a value at the end is > >> circumstantial overlap. While executing an AFTER, you wouldn't *know* > >> that you were adding it to the end, necessarily. > >> > >> The other reason to have AFTER is that, in scripts, the user may not > >> have the before value handy due to context (i.e. dynamically building an > >> enum). > >> > >> Anyway, this'll still be useful with BEFORE only. I'm just convinced > >> that we'll end up adding AFTER in 9.2 or 9.3 after we get a bunch of > >> user complaints and questions. So why not add it now? > > CREATE ENUM in PG 9.0 allows you to create an enum with no columns, > > e.g.: > > > > test=> CREATE TYPE etest AS ENUM (); > > CREATE TYPE > > > > so I think we have to have the ability add an enum without a > > before/after. This ability was added for pg_upgrade. > > > > No we don't. pg_upgrade calls a C function. There is no support for this > at the SQL level AIUI. And the ability to add labels at arbitrary > positions in the sort order is an essential part of this feature. pg_upgrade calls a C API to add labels, but the ability to create an enum with no labels is supported at the SQL level, as I showed above. I am not saying we don't need before/after, but I am saying we need the ability to add labels without using before/after because there are no labels in an empty enum. I am not sure what you are arguing for/against. I thought we were agreed to allow before/after, and no specification too. I am just pointing out that we need the "no specification" syntax for logical as well as practical reasons. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
On 08/23/2010 07:34 PM, Bruce Momjian wrote: > I looked at the pg_upgrade ramifications of this change and it seems > some adjustments will have to be made. Right now pg_upgrade creates an > empty enum type: > > CREATE TYPE etest AS ENUM (); > > and then it calls EnumValuesCreate() to create the enum labels. > EnumValuesCreate() is called from within DefineEnum() where the enum > type is created, and that assumes the enums are always created initially > sorted. That would not be true when pg_upgrade is calling > EnumValuesCreate() directly with oid assignment as part of an upgrade. > > I think the cleanest solution would be to modify pg_dump.c so that it > creates an empty ENUM type like before, but uses the new ALTER TYPE > myenum ADD 'newlabel' syntax to add the enum labels (with oid assignment > like we do for CREATE TYPE, etc.) The existing code had to hack to call > EnumValuesCreate() but with this new syntax it will no longer be > necessary. The call to EnumValuesCreate() for enums is the only time > pg_upgrade_support calls into a function rather than just assigning an > oid to a global variable, so it would be great to remove that last > direct function call usage. > I've just been taking another look at this suggestion. I think it will work quite cleanly. As long as we add the enums in the correct order it should just do the Right Thing (tm). To answer your other question, Oid wraparound will not be a problem. Will get coding. cheers andrew
On 08/25/2010 03:29 AM, Andrew Dunstan wrote: > > > I've just been taking another look at this suggestion. I think it will > work quite cleanly. As long as we add the enums in the correct order > it should just do the Right Thing (tm). > > To answer your other question, Oid wraparound will not be a problem. > > Will get coding. > > Revised patch with pg_dump and pg_upgrade support is attached. cheers andrew
Attachment
On 08/26/2010 05:24 AM, Andrew Dunstan wrote: > > > On 08/25/2010 03:29 AM, Andrew Dunstan wrote: >> >> >> I've just been taking another look at this suggestion. I think it >> will work quite cleanly. As long as we add the enums in the correct >> order it should just do the Right Thing (tm). >> >> To answer your other question, Oid wraparound will not be a problem. >> >> Will get coding. >> >> > > Revised patch with pg_dump and pg_upgrade support is attached. > > > This time in context diff format. cheers andrew
Attachment
Attached is a a slightly updated version of this with the bitrot fixed. cheers andrew
Attachment
On 29 September 2010 20:46, Andrew Dunstan <andrew@dunslane.net> wrote: > > Attached is a a slightly updated version of this with the bitrot fixed. > > cheers > > andrew > Hi, I had a quick look at this last night. I haven't had time to give it a full review, but I did spot a couple of things: 1). It still has no docs. 2). In enum_ccmp(), when you cache the full list of elements, you're not updating mycache->sort_list_length, so it will keep fetching the full list each time. Also, I think that function could use a few more comments. 3). I think you need to update psql so that \dT+ returns the enum elements in the right order. Otherwise I like it, and I definitely prefer the flexibility that this syntax gives. Regards, Dean
On 10/01/2010 04:35 AM, Dean Rasheed wrote: > 2). In enum_ccmp(), when you cache the full list of elements, you're > not updating mycache->sort_list_length, so it will keep fetching the > full list each time. Also, I think that function could use a few more > comments. Good catch. Will fix. > 3). I think you need to update psql so that \dT+ returns the enum > elements in the right order. Yeah. Will do. I will post a revised patch soon. cheers andrew
On Fri, Oct 1, 2010 at 7:12 AM, Andrew Dunstan <andrew@dunslane.net> wrote: > On 10/01/2010 04:35 AM, Dean Rasheed wrote: >> >> 2). In enum_ccmp(), when you cache the full list of elements, you're >> not updating mycache->sort_list_length, so it will keep fetching the >> full list each time. Also, I think that function could use a few more >> comments. > > Good catch. Will fix. > >> 3). I think you need to update psql so that \dT+ returns the enum >> elements in the right order. > > Yeah. Will do. > > I will post a revised patch soon. Should we postpone this to the next CommitFest? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 10/13/2010 02:08 AM, Robert Haas wrote: > On Fri, Oct 1, 2010 at 7:12 AM, Andrew Dunstan<andrew@dunslane.net> wrote: >> On 10/01/2010 04:35 AM, Dean Rasheed wrote: >>> 2). In enum_ccmp(), when you cache the full list of elements, you're >>> not updating mycache->sort_list_length, so it will keep fetching the >>> full list each time. Also, I think that function could use a few more >>> comments. >> Good catch. Will fix. >> >>> 3). I think you need to update psql so that \dT+ returns the enum >>> elements in the right order. >> Yeah. Will do. >> >> I will post a revised patch soon. > Should we postpone this to the next CommitFest? Sorry, got distracted. Here's a new patch that fixes the above and also contains some documentation. cheers andrew
Attachment
On 13 October 2010 23:17, Robert Haas <robertmhaas@gmail.com> wrote: > On Wed, Oct 13, 2010 at 7:33 AM, Andrew Dunstan <andrew@dunslane.net> wrote: >> Sorry, got distracted. Here's a new patch that fixes the above and also >> contains some documentation. > > Someone want to review this and (hopefully) mark it Ready for > Committer? I see that Brendan Jurd is the reviewer of record in the > CF app, but it seems Dean Rasheed is the person who has actually > reviewed it recently. Either way... > I'm happy to take another look at it, but I'm short on time, so I doubt that I be able to do anything before the weekend. If anyone wants to jump in before then, feel free. Regards, Dean > -- > Robert Haas > EnterpriseDB: http://www.enterprisedb.com > The Enterprise PostgreSQL Company >
On Wed, Oct 13, 2010 at 7:33 AM, Andrew Dunstan <andrew@dunslane.net> wrote: > Sorry, got distracted. Here's a new patch that fixes the above and also > contains some documentation. Someone want to review this and (hopefully) mark it Ready for Committer? I see that Brendan Jurd is the reviewer of record in the CF app, but it seems Dean Rasheed is the person who has actually reviewed it recently. Either way... -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 14 October 2010 08:39, Dean Rasheed <dean.a.rasheed@gmail.com> wrote: >> Someone want to review this and (hopefully) mark it Ready for >> Committer? I see that Brendan Jurd is the reviewer of record in the >> CF app, but it seems Dean Rasheed is the person who has actually >> reviewed it recently. Either way... >> > > I'm happy to take another look at it, but I'm short on time, so I > doubt that I be able to do anything before the weekend. If anyone > wants to jump in before then, feel free. > I started looking at this last night, but ran out of time. I'll continue this evening / over the weekend. Here are my comments so far: Patch applies cleanly to current git master with no offsets. Compiles cleanly with no warnings. Regression tests pass. The regression tests look reasonable, but I'd like to see a test of \dT+. Also it could be made to exercise the comparison function more if the test query did an ORDER BY CAST(enumlabel as planets). The docs for ALTER TYPE have been updated. I found a few minor typos, and also a couple of other sections of the manual that needed updating. Attached is an updated version with these changes. Andrew, let me know if you're happy with these tweaks and I'll continue reviewing over the weekend. Regards, Dean
Attachment
On 10/15/2010 04:33 AM, Dean Rasheed wrote: > I started looking at this last night, but ran out of time. I'll > continue this evening / over the weekend. Here are my comments so far: > > Patch applies cleanly to current git master with no offsets. > Compiles cleanly with no warnings. > Regression tests pass. > > The regression tests look reasonable, but I'd like to see a test of > \dT+. Also it could be made to exercise the comparison function more > if the test query did an ORDER BY CAST(enumlabel as planets). > > The docs for ALTER TYPE have been updated. I found a few minor typos, > and also a couple of other sections of the manual that needed > updating. > > Attached is an updated version with these changes. Andrew, let me know > if you're happy with these tweaks and I'll continue reviewing over the > weekend. Thanks for the review. This looks good. cheers andrew
> On 10/15/2010 04:33 AM, Dean Rasheed wrote:
>>
>> I started looking at this last night, but ran out of time. I'll
>> continue this evening / over the weekend.
Continuing my review of this patch...
Usability review
----------------
What the patch does:
This patch adds syntax to allow additional enum values to be added to
an enum type, at any position in the list. The syntax has already been
discussed and a broad consensus reached. To me the new syntax seems
very logical and easy to use/remember.
Do we want that?
Yes, I think so. I can think of a few past projects where I could have
used this. A possible future extension would be the ability to remove
enum values, but that is likely to have additional complications, and
I think the number of use cases for it is smaller. I see no reason to
insist that it be part of this patch.
Do we already have it?
No.
Does it follow SQL spec, or the community-agreed behavior?
Not in the SQL spec, but it does follow agreed behaviour.
Does it include pg_dump support (if applicable)?
Yes.
Are there dangers?
None that I can think of.
Have all the bases been covered?
I just noticed that a couple of columns have been added to pg_type, so
there's another bit documentation that needs updating.
Feature test
------------
Does the feature work as advertised?
Yes.
Are there corner cases the author has failed to consider?
None that I could find.
Are there any assertion failures or crashes?
Yes, I'm afraid I managed to provoke a crash by running the regression
tests with -DCATCACHE_FORCE_RELEASE, after spotting a suspicious bit
of code (see below).
Performance review
------------------
Does the patch slow down simple tests?
There is a documented performance penalty when working with enum
values that are not in OID order. To attempt to measure this I created
2 tables, foo and bar, each with 800,000 rows. foo had an enum column
using the planets enum from the regression test, with values not in
OID order. bar had a similar enum column, but with values in the
default OID order.
Performance differences for comparison-heavy operations are most noticable:
SELECT MAX(p) FROM foo; max
---------neptune
(1 row)
Time: 132.305 ms
SELECT MAX(p) FROM bar; max
---------neptune
(1 row)
Time: 93.313 ms
SELECT p FROM foo ORDER BY p OFFSET 500000 LIMIT 1; p
--------saturn
(1 row)
Time: 1516.725 ms
SELECT p FROM bar ORDER BY p OFFSET 500000 LIMIT 1; p
--------saturn
(1 row)
Time: 1043.010 ms
(optimised build, asserts off)
That's a bigger performance hit than a I would have expected, even
though it is documented.
enum_ccmp() is using bsearch(), so there's a fair amount of function
call overhead there, and perhaps for small enums, a simple linear scan
would be faster. I'm not sure to what extent this is worth worrying
about.
Code review
-----------
I'm not familar enough with the pg_upgrade code to really comment on it.
Looking at AddEnumLabel() I found it a bit hard to follow, but near
the end of the block used when BEFORE/AFTER is specified, it does
this:
ReleaseCatCacheList(list);
/* are the labels sorted by OID? */ if (result && newelemorder > 1) result = newOid >
HeapTupleGetOid(existing[newelemorder-2]); if (result && newelemorder < nelems + 1) result = newOid
<HeapTupleGetOid(existing[newelemorder-1]);
It looks to me as though 'existing[...]' is a pointer into a member of
the list that's just been freed, so it risks reading freed memory.
That seems to be confirmed by running the tests with
-DCATCACHE_FORCE_RELEASE. Doing so causes a number of the tests to
fail/crash, but I didn't dig any deeper to confirm that this was the
cause.
For the most part, this patch looks good, but I think there is still a
bit of tidying up to do.
Regards,
Dean
On 16 October 2010 18:25, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> Are there corner cases the author has failed to consider?
>
> None that I could find.
>
> Are there any assertion failures or crashes?
>
I just thought of another corner case, which can lead to a crash. The
comparison code assumes that the number of elements in the enumeration
is constant during a query, but that's not necessarily the case. For
example the following will crash it:
CREATE TYPE test_enum AS ENUM('Elem 1');
CREATE OR REPLACE FUNCTION test_fn(a int) RETURNS test_enum AS
$$
DECLARE new_label text;
BEGIN new_label := 'Elem '||a; EXECUTE 'ALTER TYPE test_enum ADD '''||new_label||''' BEFORE ''Elem 1'''; RETURN
new_label::test_enum;
END;
$$
LANGUAGE plpgsql;
WITH t(a) AS (SELECT test_fn(i) FROM generate_series(2, 10) g(i)) SELECT MAX(a) from t;
Of course that's a pathalogical example, but we should protect against
it, preferrably without compromising performance in more normal cases.
Regards,
Dean
On 10/17/2010 05:30 AM, Dean Rasheed wrote:
> On 16 October 2010 18:25, Dean Rasheed<dean.a.rasheed@gmail.com> wrote:
>> Are there corner cases the author has failed to consider?
>>
>> None that I could find.
>>
>> Are there any assertion failures or crashes?
>>
> I just thought of another corner case, which can lead to a crash. The
> comparison code assumes that the number of elements in the enumeration
> is constant during a query, but that's not necessarily the case. For
> example the following will crash it:
>
> CREATE TYPE test_enum AS ENUM('Elem 1');
>
> CREATE OR REPLACE FUNCTION test_fn(a int) RETURNS test_enum AS
> $$
> DECLARE
> new_label text;
> BEGIN
> new_label := 'Elem '||a;
> EXECUTE 'ALTER TYPE test_enum ADD '''||new_label||''' BEFORE ''Elem 1''';
> RETURN new_label::test_enum;
> END;
> $$
> LANGUAGE plpgsql;
>
> WITH t(a) AS (SELECT test_fn(i) FROM generate_series(2, 10) g(i))
> SELECT MAX(a) from t;
>
> Of course that's a pathalogical example, but we should protect against
> it, preferrably without compromising performance in more normal cases.
Yeah, good point. But how do we manage that? Looking up the number of
elements on each function call will cause a performance degradation, I
suspect. I'll think about it, but if you have any ideas please speak up.
I'm fairly sure we should also recheck the cached sorted property for
the same reason, incidentally.
cheers
andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> On 10/17/2010 05:30 AM, Dean Rasheed wrote:
>> I just thought of another corner case, which can lead to a crash. The
>> comparison code assumes that the number of elements in the enumeration
>> is constant during a query, but that's not necessarily the case.
>> ...
>> Of course that's a pathalogical example, but we should protect against
>> it, preferrably without compromising performance in more normal cases.
> Yeah, good point. But how do we manage that?
Why is it crashing? I can see that this sort of thing might lead to
nonsensical answers, but a crash is harder to understand.
regards, tom "haven't read the patch" lane
On 17 October 2010 15:38, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Andrew Dunstan <andrew@dunslane.net> writes: >> On 10/17/2010 05:30 AM, Dean Rasheed wrote: >>> I just thought of another corner case, which can lead to a crash. The >>> comparison code assumes that the number of elements in the enumeration >>> is constant during a query, but that's not necessarily the case. >>> ... >>> Of course that's a pathalogical example, but we should protect against >>> it, preferrably without compromising performance in more normal cases. > >> Yeah, good point. But how do we manage that? > > Why is it crashing? I can see that this sort of thing might lead to > nonsensical answers, but a crash is harder to understand. > Hmm, it's harder than I thought. The crash is because each time it finds a label it hasn't seen before it adds it to the array of cached values without checking the array bounds. That array is only as big as the number of elements in the enum the first time it was called. Allowing the array to grow would prevent crashes, but not protect again returning incorrect answers. Perhaps it should just read and cache all the values the first time it is called. Then if it ever fails to find a value in the array, it knows that the enum must have grown, and it can rebuild the whole array. Regards, Dean > regards, tom "haven't read the patch" lane >
On 10/17/2010 10:38 AM, Tom Lane wrote: > Andrew Dunstan<andrew@dunslane.net> writes: >> On 10/17/2010 05:30 AM, Dean Rasheed wrote: >>> I just thought of another corner case, which can lead to a crash. The >>> comparison code assumes that the number of elements in the enumeration >>> is constant during a query, but that's not necessarily the case. >>> ... >>> Of course that's a pathalogical example, but we should protect against >>> it, preferrably without compromising performance in more normal cases. >> Yeah, good point. But how do we manage that? > Why is it crashing? I can see that this sort of thing might lead to > nonsensical answers, but a crash is harder to understand. > > regards, tom "haven't read the patch" lane Heh. I've been deep in buildfarm work, but I'll look at this now to see what I can find. cheers andrew
Dean Rasheed <dean.a.rasheed@gmail.com> writes:
> Hmm, it's harder than I thought. The crash is because each time it
> finds a label it hasn't seen before it adds it to the array of cached
> values without checking the array bounds. That array is only as big as
> the number of elements in the enum the first time it was called.
[ scratches head... ] And where does it get that number of elements
from, if not by doing the same work that would allow it to fill the
array completely? Something seems ill-designed here.
> Allowing the array to grow would prevent crashes, but not protect
> again returning incorrect answers.
Well, one of the questions here is exactly how wrong the answers can
be. Offhand, it seems to me that comparisons of two existing entries
can never be falsified by adding a new entry, so I'm not seeing that
there could be any real problem. If we allowed rearrangement of the
sort order of existing entries, it'd be problematic.
> Perhaps it should just read and cache all the values the first time it
> is called. Then if it ever fails to find a value in the array, it
> knows that the enum must have grown, and it can rebuild the whole
> array.
This is kept in typcache, right? ISTM the right thing to do is arrange
to invalidate the cached array when a cache flush event occurs, and
rebuild the whole array on next use. Then you just throw an error if
you're passed a value that isn't there.
regards, tom lane
On 17 October 2010 16:49, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Dean Rasheed <dean.a.rasheed@gmail.com> writes: >> Hmm, it's harder than I thought. The crash is because each time it >> finds a label it hasn't seen before it adds it to the array of cached >> values without checking the array bounds. That array is only as big as >> the number of elements in the enum the first time it was called. > > [ scratches head... ] And where does it get that number of elements > from, if not by doing the same work that would allow it to fill the > array completely? Something seems ill-designed here. > Hmm. That's coming from a new column added to pg_type (typnlabels). Perhaps that's not safe though. Are there potential race conditions there? >> Allowing the array to grow would prevent crashes, but not protect >> again returning incorrect answers. > > Well, one of the questions here is exactly how wrong the answers can > be. Offhand, it seems to me that comparisons of two existing entries > can never be falsified by adding a new entry, so I'm not seeing that > there could be any real problem. If we allowed rearrangement of the > sort order of existing entries, it'd be problematic. > >> Perhaps it should just read and cache all the values the first time it >> is called. Then if it ever fails to find a value in the array, it >> knows that the enum must have grown, and it can rebuild the whole >> array. > > This is kept in typcache, right? ISTM the right thing to do is arrange > to invalidate the cached array when a cache flush event occurs, and > rebuild the whole array on next use. Then you just throw an error if > you're passed a value that isn't there. > Makes sense. Regards, Dean > regards, tom lane >
Dean Rasheed <dean.a.rasheed@gmail.com> writes:
> On 17 October 2010 16:49, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> [ scratches head... ] �And where does it get that number of elements
>> from, if not by doing the same work that would allow it to fill the
>> array completely? �Something seems ill-designed here.
> Hmm. That's coming from a new column added to pg_type (typnlabels).
> Perhaps that's not safe though. Are there potential race conditions
> there?
I knew I shoulda read this patch ;-). That seems a lot more invasive
than this feature justifies. And I share your qualms about whether it's
race-condition-proof. We don't have very much locking on pg_type
entries, so making a hard assumption about consistency between two
different catalogs seems pretty risky.
The way I'd be inclined to design this is that altering an enum doesn't
change its pg_type entry at all, just add another row to pg_enum.
When first needing to compare values of an enum, load up the typcache
entry for it. This involves scanning all the entries for that type OID
in pg_enum, and determining from that whether you can compare the easy
way or not. If not, build the array that tells you how to sort, and put
it in the typcache entry.
The missing piece in this is how to determine when the typcache entry
has to be flushed. That should be driven by sinval signalling. There
are two different ways you could do it:
1. Watch for SI events on pg_enum. The problem here is we don't have
a syscache on pg_enum, and there's no obvious other reason to want one.
Also I think you'd have to flush *all* enum typcache entries, since you
couldn't always tell which one had been modified.
2. Watch for SI events on the pg_type row. However, since according
to what I said above an ALTER TYPE wouldn't actually modify the pg_type
row, you'd need to have the ALTER send out a "dummy" SI event. This
is not hard to do --- we have the concept of dummy inval events on
relations already --- but it's a bit ugly because of the risk of
omission. But the number of places that would need to do this seems
small, so I don't think that risk is large.
On balance I like the second idea better.
regards, tom lane
I wrote:
> The missing piece in this is how to determine when the typcache entry
> has to be flushed. That should be driven by sinval signalling.
On reflection that doesn't seem good enough. Immediately after someone
else has committed an ALTER TYPE, your typcache entry is out of date,
and won't be updated until you get around to noticing the SI signal.
I was thinking that wouldn't matter because you'd never need to make a
comparison involving the new enum value --- it couldn't be in any table
rows you'd see as committed good. But this is wrong because you might
have to make index comparisons involving the new value, even before you
consider that the rows the index entries reference are good.
We could fix that with Dean's idea of reloading the cache whenever
we see that we are being asked to compare a value we don't have in the
cache entry. However, that assumes that we even notice that it's not
in the cache entry. If we're trying to use "fast" comparison then we
wouldn't notice that.
So the hard part of this really is to force other backends to switch
from "fast" to "slow" comparison in a timely fashion when an ALTER makes
that necessary. Right offhand I don't see any good way to do that,
at least not while having the "fast" method as fast as it is now.
regards, tom lane
On 17 October 2010 18:53, Tom Lane <tgl@sss.pgh.pa.us> wrote: > I wrote: >> The missing piece in this is how to determine when the typcache entry >> has to be flushed. That should be driven by sinval signalling. > > On reflection that doesn't seem good enough. Immediately after someone > else has committed an ALTER TYPE, your typcache entry is out of date, > and won't be updated until you get around to noticing the SI signal. > I was thinking that wouldn't matter because you'd never need to make a > comparison involving the new enum value --- it couldn't be in any table > rows you'd see as committed good. But this is wrong because you might > have to make index comparisons involving the new value, even before you > consider that the rows the index entries reference are good. > > We could fix that with Dean's idea of reloading the cache whenever > we see that we are being asked to compare a value we don't have in the > cache entry. However, that assumes that we even notice that it's not > in the cache entry. If we're trying to use "fast" comparison then we > wouldn't notice that. > That makes me think maybe the "fast" and "slow" comparisons should both be done the same way, having a cache so that we notice immediately if we get a new value. Obviously that's not going to be as fast as the current "fast" method, but the question is, can it be made sufficiently close? I think the current sort+bsearch method is always going to be significantly slower, but perhaps a dedicated hash table algorithm might work. Regards, Dean > So the hard part of this really is to force other backends to switch > from "fast" to "slow" comparison in a timely fashion when an ALTER makes > that necessary. Right offhand I don't see any good way to do that, > at least not while having the "fast" method as fast as it is now. > > regards, tom lane >
On 10/17/2010 01:20 PM, Tom Lane wrote: > I knew I shoulda read this patch ;-). That seems a lot more invasive > than this feature justifies. And I share your qualms about whether it's > race-condition-proof. We don't have very much locking on pg_type > entries, so making a hard assumption about consistency between two > different catalogs seems pretty risky. > > The way I'd be inclined to design this is that altering an enum doesn't > change its pg_type entry at all, just add another row to pg_enum. > When first needing to compare values of an enum, load up the typcache > entry for it. This involves scanning all the entries for that type OID > in pg_enum, and determining from that whether you can compare the easy > way or not. If not, build the array that tells you how to sort, and put > it in the typcache entry. Perhaps mistakenly I wanted to avoid doing that as it would slow down a retail comparison quite a lot, especially in the case of an enum with a very large label set. That's why I put the sorted property and label count in pg_type. cheers andrew
Dean Rasheed <dean.a.rasheed@gmail.com> writes:
> On 17 October 2010 18:53, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> We could fix that with Dean's idea of reloading the cache whenever
>> we see that we are being asked to compare a value we don't have in the
>> cache entry. �However, that assumes that we even notice that it's not
>> in the cache entry. �If we're trying to use "fast" comparison then we
>> wouldn't notice that.
> That makes me think maybe the "fast" and "slow" comparisons should
> both be done the same way, having a cache so that we notice
> immediately if we get a new value.
Actually ... the race conditions can be a lot worse than just a race.
Consider
begin;alter type myenum add 'some-value';insert into mytab values('some-value');rollback;
If mytab has an index on the enum col, we now have an index entry that
contains an enum value that isn't valid according to anybody, and nobody
knows how to compare it. If that entry is near the root then the index
is hopelessly corrupt: no one can tell which way to descend when
comparing it to some valid value.
I think what this says is that we cannot allow any manipulations that
involve an uncommitted enum value. Probably the easiest way is to make
the ALTER TYPE operation disallowed-inside-transaction-block. That's
pretty ugly, but doesn't seem like a serious restriction in practice
(though for example it'd mean we couldn't use it in pg_dump).
I'm not sure if enforcing such a restriction helps much in terms of
managing cache invalidations. Even with that, it seems possible to
encounter index entries for values that you haven't yet noticed the
invalidation message for.
regards, tom lane
On 10/17/2010 02:19 PM, Dean Rasheed wrote: > That makes me think maybe the "fast" and "slow" comparisons should > both be done the same way, having a cache so that we notice > immediately if we get a new value. > > Obviously that's not going to be as fast as the current "fast" method, > but the question is, can it be made sufficiently close? I think the > current sort+bsearch method is always going to be significantly > slower, but perhaps a dedicated hash table algorithm might work. > Making that as fast as "Is this sorted? If yes, compare the two oids" or even acceptably slower seems likely to be a challenge. I thought about the sort of approach you suggest initially and didn't come up with anything that seemed likely to work well enough. cheers andrew
On 10/17/2010 03:17 PM, Tom Lane wrote:
> Dean Rasheed<dean.a.rasheed@gmail.com> writes:
>> On 17 October 2010 18:53, Tom Lane<tgl@sss.pgh.pa.us> wrote:
>>> We could fix that with Dean's idea of reloading the cache whenever
>>> we see that we are being asked to compare a value we don't have in the
>>> cache entry. However, that assumes that we even notice that it's not
>>> in the cache entry. If we're trying to use "fast" comparison then we
>>> wouldn't notice that.
>> That makes me think maybe the "fast" and "slow" comparisons should
>> both be done the same way, having a cache so that we notice
>> immediately if we get a new value.
> Actually ... the race conditions can be a lot worse than just a race.
> Consider
>
> begin;
> alter type myenum add 'some-value';
> insert into mytab values('some-value');
> rollback;
>
> If mytab has an index on the enum col, we now have an index entry that
> contains an enum value that isn't valid according to anybody, and nobody
> knows how to compare it. If that entry is near the root then the index
> is hopelessly corrupt: no one can tell which way to descend when
> comparing it to some valid value.
>
> I think what this says is that we cannot allow any manipulations that
> involve an uncommitted enum value. Probably the easiest way is to make
> the ALTER TYPE operation disallowed-inside-transaction-block. That's
> pretty ugly, but doesn't seem like a serious restriction in practice
> (though for example it'd mean we couldn't use it in pg_dump).
Even in binary upgrade mode?
cheers
andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> Making that as fast as "Is this sorted? If yes, compare the two oids" or
> even acceptably slower seems likely to be a challenge. I thought about
> the sort of approach you suggest initially and didn't come up with
> anything that seemed likely to work well enough.
The fundamental problem here is that we can't tell by examining an enum
value whether we have to apply the "fast" or "slow" comparison method.
But what if we could?
The sneaky idea that just came to me is to declare that even-numbered
OID values can be sorted by direct comparison, whereas odd-numbered OIDs
can't. It seems fairly easy to ensure that that property holds while
creating the values, as long as you don't mind "burning" OIDs: if you
get a value you don't like, just demand another one. Then, as long as
both OIDs involved in a comparison are even, you just do a direct
comparison and no cache entry is needed at all. When either is odd, you
know you need a cache entry. You can also tell whether an existing
cache entry is stale: if it doesn't contain both values then you need to
refresh it. If it does have both, then it's good enough for the
immediate purpose, even if there are other values it doesn't know
about. So with this design we don't actually have to watch for inval
events at all ... we just refresh the cache entry whenever a comparison
finds that that's needed.
The one problem I can see with this is that it's only partially
on-disk-compatible with existing enum types: it'll almost certainly
think that they require slow comparison, even when they don't.
Maybe that's acceptable.
regards, tom lane
Andrew Dunstan <andrew@dunslane.net> writes:
> On 10/17/2010 03:17 PM, Tom Lane wrote:
>> I think what this says is that we cannot allow any manipulations that
>> involve an uncommitted enum value. Probably the easiest way is to make
>> the ALTER TYPE operation disallowed-inside-transaction-block. That's
>> pretty ugly, but doesn't seem like a serious restriction in practice
>> (though for example it'd mean we couldn't use it in pg_dump).
> Even in binary upgrade mode?
Binary upgrade can probably be treated as a special case.
regards, tom lane
Andrew Dunstan <andrew@dunslane.net> writes:
> On 10/17/2010 01:20 PM, Tom Lane wrote:
>> The way I'd be inclined to design this is that altering an enum doesn't
>> change its pg_type entry at all, just add another row to pg_enum.
>> When first needing to compare values of an enum, load up the typcache
>> entry for it.
> Perhaps mistakenly I wanted to avoid doing that as it would slow down a
> retail comparison quite a lot, especially in the case of an enum with a
> very large label set. That's why I put the sorted property and label
> count in pg_type.
Just going back to this point: I don't buy that argument at all.
If you have to consult pg_type to find out whether fast or slow
comparison is needed, you've already burned all the cycles required
for a cache lookup. The only way that a large typcache entry would
really be a performance issue compared to just consulting pg_type
is if it had to be refreshed a lot, which doesn't seem like a likely
problem.
regards, tom lane
<br /><br /> On 10/17/2010 03:56 PM, Tom Lane wrote: <blockquote cite="mid:13685.1287345367@sss.pgh.pa.us" type="cite"><prewrap="">Andrew Dunstan <a class="moz-txt-link-rfc2396E" href="mailto:andrew@dunslane.net"><andrew@dunslane.net></a>writes: </pre><blockquote type="cite"><pre wrap="">Making that as fast as "Is this sorted? If yes, compare the two oids" or even acceptably slower seems likely to be a challenge. I thought about the sort of approach you suggest initially and didn't come up with anything that seemed likely to work well enough. </pre></blockquote><pre wrap=""> The fundamental problem here is that we can't tell by examining an enum value whether we have to apply the "fast" or "slow" comparison method. But what if we could? The sneaky idea that just came to me is to declare that even-numbered OID values can be sorted by direct comparison, whereas odd-numbered OIDs can't. It seems fairly easy to ensure that that property holds while creating the values, as long as you don't mind "burning" OIDs: if you get a value you don't like, just demand another one. Then, as long as both OIDs involved in a comparison are even, you just do a direct comparison and no cache entry is needed at all. When either is odd, you know you need a cache entry. You can also tell whether an existing cache entry is stale: if it doesn't contain both values then you need to refresh it. If it does have both, then it's good enough for the immediate purpose, even if there are other values it doesn't know about. So with this design we don't actually have to watch for inval events at all ... we just refresh the cache entry whenever a comparison finds that that's needed. </pre></blockquote><br /> Hmm, nice. What I like about this is that it's not an all or nothing deal. If you add one labelthat needs an odd Oid, most of the comparisons will still be fast.<br /><br /> I think the rule for choosing the Oidfor the new entry would go something like this:<br /><ul><li>if we're adding a label at the end and the Oid of the lastentry is even, take the first Oid that is either even and greater than the Oid of the last entry, or odd and less thanthe Oid of the last entry<li>for all other positions take the first odd Oid</ul><br /><br /><blockquote cite="mid:13685.1287345367@sss.pgh.pa.us"type="cite"><pre wrap=""> The one problem I can see with this is that it's only partially on-disk-compatible with existing enum types: it'll almost certainly think that they require slow comparison, even when they don't. Maybe that's acceptable. </pre></blockquote><br /><br /><br /> Yeah, not sure about that. <br /><br /> cheers<br /><br /> andrew<br />
On Sun, Oct 17, 2010 at 12:17 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> begin;
> alter type myenum add 'some-value';
> insert into mytab values('some-value');
> rollback;
....
> I think what this says is that we cannot allow any manipulations that
> involve an uncommitted enum value. Probably the easiest way is to make
> the ALTER TYPE operation disallowed-inside-transaction-block. That's
> pretty ugly, but doesn't seem like a serious restriction in practice
> (though for example it'd mean we couldn't use it in pg_dump).
I fear this is the camel's nose under the door towards making all DDL
non-transactional a la Oracle.
The alternative is that there are two steps to creating an enum. A
low-level modification which adds the new value and its collation
value to the list of valid things to compare. This would be done as
something like an autonomous transaction and be committed regardless
of whether the outer transaction commits. It wouldn't add the value to
the user-visible list of values or allowable values for inserting.
Only once that was committed could we then make the transactional
modification to the user visible DDL.
The main problem with this is of course that we don't actually have
autonomous transactions...
--
greg
On 10/17/2010 03:56 PM, Tom Lane wrote: [clever scheme to treat even numbered enum Oids as sorted] > The one problem I can see with this is that it's only partially > on-disk-compatible with existing enum types: it'll almost certainly > think that they require slow comparison, even when they don't. > Maybe that's acceptable. Thinking more about this, could we do some sort of hybrid scheme that stashes the 'oids are sorted correctly' property of the type somewhat like what I proposed? Binary upgrade would be mildly tricky but not impossible. cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> On 10/17/2010 03:56 PM, Tom Lane wrote:
>> [clever scheme to treat even numbered enum Oids as sorted]
>> The one problem I can see with this is that it's only partially
>> on-disk-compatible with existing enum types: it'll almost certainly
>> think that they require slow comparison, even when they don't.
>> Maybe that's acceptable.
> Thinking more about this, could we do some sort of hybrid scheme that
> stashes the 'oids are sorted correctly' property of the type somewhat
> like what I proposed?
I think that fundamentally doesn't work in the face of a concurrent
ALTER TYPE. And even if it did work, by the time you've done the
cache lookup to check your stashed flag, what have you really saved?
The only way to have comparisons that are on par with current
performance is to not need to do any lookup at all.
The scenario that seems the most dangerous to me is
1. Begin transaction T1.2. T1 does cache lookup, or any other pushup you want, and decides that the enum type is
correctlysorted.3. T2 commits an ALTER TYPE that adds a non-sorted OID.4. T3 inserts that OID in an index.5. T1
encountersthe out-of-order OID in the index.
If T1 is not able to recognize that the OID it's looking at is not
in-order, despite its previously cached information, it's going to lose
badly. It's impractical to do an AcceptInvalidationMessages on every
single comparison, so AFAICT you *have to* be able to deal correctly
with enum values that were added since your cache entry was made.
We could possibly deal with enum types that follow the existing
convention if we made the cache entry hold a list of all the original,
known-to-be-sorted OIDs. (This could be reasonably compact and cheap to
probe if it were represented as a starting OID and a Bitmapset of delta
values, since we can assume that the initial set of OIDs is pretty close
together.) But we have to have that cache entry, and we have to consult
it on every single comparison, so it's definitely going to be slower
than before.
So I'm thinking the comparison procedure goes like this:
1. Both OIDs even?If so, just compare them numerically, and we're done.
2. Lookup cache entry for enum type.
3. Both OIDs in list of known-sorted OIDs?If so, just compare them numerically, and we're done.
4. Search the part of the cache entry that lists sort positions.If not both present, refresh the cache entry.If still
notpresent, throw error.
5. Compare by sort positions.
Step 4 is the slowest part but would be avoided in most cases.
However, step 2 is none too speedy either, and would usually
be required when dealing with pre-existing enums.
regards, tom lane
On 10/18/2010 10:52 AM, Tom Lane wrote: > We could possibly deal with enum types that follow the existing > convention if we made the cache entry hold a list of all the original, > known-to-be-sorted OIDs. (This could be reasonably compact and cheap to > probe if it were represented as a starting OID and a Bitmapset of delta > values, since we can assume that the initial set of OIDs is pretty close > together.) How are we going to know what those are? We could keep track of the beginning and end of the original range maybe and refuse to create any new enum values for the type inside that range. That might make binary upgrade a bit ugly, but it's probably manageable even so. cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> On 10/18/2010 10:52 AM, Tom Lane wrote:
>> We could possibly deal with enum types that follow the existing
>> convention if we made the cache entry hold a list of all the original,
>> known-to-be-sorted OIDs. (This could be reasonably compact and cheap to
>> probe if it were represented as a starting OID and a Bitmapset of delta
>> values, since we can assume that the initial set of OIDs is pretty close
>> together.)
> How are we going to know what those are?
You read pg_enum while constructing the cache entry, sort by nominal
sort position, and look to see how many OIDs form a sorted and reasonably
compact range. I don't see a need to know which of those OIDs were
actually original; you can get close enough with heuristic reverse
engineering. The only real limitation is not wanting the bitmapset to
get too enormous, so you might often be able to incorporate OIDs that
in fact weren't original, so long as they chanced to be OK order-wise.
This'd be a win anytime it saved you from having to proceed to step 4
in comparisons.
regards, tom lane
On 18 October 2010 15:52, Tom Lane <tgl@sss.pgh.pa.us> wrote: > So I'm thinking the comparison procedure goes like this: > > 1. Both OIDs even? > If so, just compare them numerically, and we're done. > > 2. Lookup cache entry for enum type. > > 3. Both OIDs in list of known-sorted OIDs? > If so, just compare them numerically, and we're done. > > 4. Search the part of the cache entry that lists sort positions. > If not both present, refresh the cache entry. > If still not present, throw error. > > 5. Compare by sort positions. > > Step 4 is the slowest part but would be avoided in most cases. > However, step 2 is none too speedy either, and would usually > be required when dealing with pre-existing enums. > I was thinking that steps 2-5 could be sped up by doing something like: 2. If first time in: Build a lightweight hash map: [ oid -> sort position ] with all the enum values 3. Look up each oid in the hash map If not both present, rebuild the hash map If still not present, throw error. 4. Compare by sort positions. I think the hash map lookups could be made pretty quick, if we're prepared to write a bit of dedicated code there rather than relying on the more general-purpose caching code. Regards, Dean > regards, tom lane >
Dean Rasheed <dean.a.rasheed@gmail.com> writes:
> I think the hash map lookups could be made pretty quick, if we're
> prepared to write a bit of dedicated code there rather than relying on
> the more general-purpose caching code.
It's still going to be very significantly slower than what I'm
suggesting --- I'm *already* assuming that step 4 is using a hash
or something else smarter than just traversing a list. My step 3
is just a bit-array index operation (well, 2 of 'em).
(I'm pretty dubious of unsubstantiated claims that you can build a hash
table that's significantly faster than our existing hash code, anyway.)
regards, tom lane
On 10/18/2010 01:40 PM, Dean Rasheed wrote: > On 18 October 2010 15:52, Tom Lane<tgl@sss.pgh.pa.us> wrote: >> So I'm thinking the comparison procedure goes like this: >> >> 1. Both OIDs even? >> If so, just compare them numerically, and we're done. >> >> 2. Lookup cache entry for enum type. >> >> 3. Both OIDs in list of known-sorted OIDs? >> If so, just compare them numerically, and we're done. >> >> 4. Search the part of the cache entry that lists sort positions. >> If not both present, refresh the cache entry. >> If still not present, throw error. >> >> 5. Compare by sort positions. >> >> Step 4 is the slowest part but would be avoided in most cases. >> However, step 2 is none too speedy either, and would usually >> be required when dealing with pre-existing enums. >> > I was thinking that steps 2-5 could be sped up by doing something like: > > 2. If first time in: > Build a lightweight hash map: [ oid -> sort position ] with > all the enum values > > 3. Look up each oid in the hash map > If not both present, rebuild the hash map > If still not present, throw error. > > 4. Compare by sort positions. > > I think the hash map lookups could be made pretty quick, if we're > prepared to write a bit of dedicated code there rather than relying on > the more general-purpose caching code. > If you have want to work on it and prove it's going to be better, please do. I'm not convinced it will do a whole lot better than a binary search that in most cases will do no more than a handful of probes. cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> If you have want to work on it and prove it's going to be better, please
> do. I'm not convinced it will do a whole lot better than a binary search
> that in most cases will do no more than a handful of probes.
Yeah, that's a good point. There's a range of table sizes where hashing
is faster than binary search, but I'm not sure that typical enums will
fall into that range.
regards, tom lane
On 10/18/2010 10:52 AM, Tom Lane wrote: > We could possibly deal with enum types that follow the existing > convention if we made the cache entry hold a list of all the original, > known-to-be-sorted OIDs. (This could be reasonably compact and cheap to > probe if it were represented as a starting OID and a Bitmapset of delta > values, since we can assume that the initial set of OIDs is pretty close > together.) But we have to have that cache entry, and we have to consult > it on every single comparison, so it's definitely going to be slower > than before. > > So I'm thinking the comparison procedure goes like this: > > 1. Both OIDs even? > If so, just compare them numerically, and we're done. > > 2. Lookup cache entry for enum type. > > 3. Both OIDs in list of known-sorted OIDs? > If so, just compare them numerically, and we're done. > > 4. Search the part of the cache entry that lists sort positions. > If not both present, refresh the cache entry. > If still not present, throw error. > > 5. Compare by sort positions. > > Step 4 is the slowest part but would be avoided in most cases. > However, step 2 is none too speedy either, and would usually > be required when dealing with pre-existing enums. OK, I've made adjustments that I think do what you're suggesting. Patch is attached. Alternatively this can be pulled from <git@github.com:adunstan/postgresql-dev.git> cheers andrew
Attachment
On 19 October 2010 05:21, Andrew Dunstan <andrew@dunslane.net> wrote:
>
>
> On 10/18/2010 10:52 AM, Tom Lane wrote:
>>
>> We could possibly deal with enum types that follow the existing
>> convention if we made the cache entry hold a list of all the original,
>> known-to-be-sorted OIDs. (This could be reasonably compact and cheap to
>> probe if it were represented as a starting OID and a Bitmapset of delta
>> values, since we can assume that the initial set of OIDs is pretty close
>> together.) But we have to have that cache entry, and we have to consult
>> it on every single comparison, so it's definitely going to be slower
>> than before.
>>
>> So I'm thinking the comparison procedure goes like this:
>>
>> 1. Both OIDs even?
>> If so, just compare them numerically, and we're done.
>>
>> 2. Lookup cache entry for enum type.
>>
>> 3. Both OIDs in list of known-sorted OIDs?
>> If so, just compare them numerically, and we're done.
>>
>> 4. Search the part of the cache entry that lists sort positions.
>> If not both present, refresh the cache entry.
>> If still not present, throw error.
>>
>> 5. Compare by sort positions.
>>
>> Step 4 is the slowest part but would be avoided in most cases.
>> However, step 2 is none too speedy either, and would usually
>> be required when dealing with pre-existing enums.
>
> OK, I've made adjustments that I think do what you're suggesting.
>
> Patch is attached.
>
Ah, I'd missed the point about the bitmapset. In the most common case,
most of the enum elements are probably going to be in the right order,
so you save a lot by identifying that case quickly.
I didn't have time to play with hash maps myself, but I don't think it
will make much difference now because hopefully the binary search
isn't going to be hit a lot anyway.
There are a couple of things that look odd about this code though (I
haven't tested it, but they look wrong):
In the loop identifying the longest range:
for (list_end = start_pos+1; list_end < num; list_end++) if
(mycache->sort_order_list[list_end].sort_order< mycache->sort_order_list[list_end - 1].sort_order ||
mycache->sort_order_list[list_end].enum_oid - mycache->sort_order_list[list_end].enum_oid >=
BITMAPSIZE)
That last test isn't doing anything. Shouldn't the second "list_end"
should be "start_pos"?
In the loop that sets bits in the bitmap, it looks like it's assuming
the range starts at 0. I haven't had any caffeine yet, so maybe I'm
misunderstanding it, but I can't see anywhere that sets bitmap_base.
Regards,
Dean
> Alternatively this can be pulled from
> <git@github.com:adunstan/postgresql-dev.git>
>
> cheers
>
> andrew
>
On 10/19/2010 04:00 AM, Dean Rasheed wrote: > There are a couple of things that look odd about this code though (I > haven't tested it, but they look wrong): You're right, thanks. I have fixed those. New patch attached. cheers andrew
Attachment
On 19 October 2010 05:21, Andrew Dunstan <andrew@dunslane.net> wrote: > > > On 10/18/2010 10:52 AM, Tom Lane wrote: >> >> We could possibly deal with enum types that follow the existing >> convention if we made the cache entry hold a list of all the original, >> known-to-be-sorted OIDs. (This could be reasonably compact and cheap to >> probe if it were represented as a starting OID and a Bitmapset of delta >> values, since we can assume that the initial set of OIDs is pretty close >> together.) But we have to have that cache entry, and we have to consult >> it on every single comparison, so it's definitely going to be slower >> than before. >> >> So I'm thinking the comparison procedure goes like this: >> >> 1. Both OIDs even? >> If so, just compare them numerically, and we're done. >> >> 2. Lookup cache entry for enum type. >> >> 3. Both OIDs in list of known-sorted OIDs? >> If so, just compare them numerically, and we're done. >> >> 4. Search the part of the cache entry that lists sort positions. >> If not both present, refresh the cache entry. >> If still not present, throw error. >> >> 5. Compare by sort positions. >> >> Step 4 is the slowest part but would be avoided in most cases. >> However, step 2 is none too speedy either, and would usually >> be required when dealing with pre-existing enums. > > OK, I've made adjustments that I think do what you're suggesting. > > Patch is attached. > > Alternatively this can be pulled from > <git@github.com:adunstan/postgresql-dev.git> Andrew, can't you get your own repo at git.postgresql.org? -- Thom Brown Twitter: @darkixion IRC (freenode): dark_ixion Registered Linux user: #516935
On Tue, Oct 19, 2010 at 10:19, Thom Brown <thom@linux.com> wrote: > On 19 October 2010 05:21, Andrew Dunstan <andrew@dunslane.net> wrote: >> >> >> On 10/18/2010 10:52 AM, Tom Lane wrote: >>> >>> We could possibly deal with enum types that follow the existing >>> convention if we made the cache entry hold a list of all the original, >>> known-to-be-sorted OIDs. (This could be reasonably compact and cheap to >>> probe if it were represented as a starting OID and a Bitmapset of delta >>> values, since we can assume that the initial set of OIDs is pretty close >>> together.) But we have to have that cache entry, and we have to consult >>> it on every single comparison, so it's definitely going to be slower >>> than before. >>> >>> So I'm thinking the comparison procedure goes like this: >>> >>> 1. Both OIDs even? >>> If so, just compare them numerically, and we're done. >>> >>> 2. Lookup cache entry for enum type. >>> >>> 3. Both OIDs in list of known-sorted OIDs? >>> If so, just compare them numerically, and we're done. >>> >>> 4. Search the part of the cache entry that lists sort positions. >>> If not both present, refresh the cache entry. >>> If still not present, throw error. >>> >>> 5. Compare by sort positions. >>> >>> Step 4 is the slowest part but would be avoided in most cases. >>> However, step 2 is none too speedy either, and would usually >>> be required when dealing with pre-existing enums. >> >> OK, I've made adjustments that I think do what you're suggesting. >> >> Patch is attached. >> >> Alternatively this can be pulled from >> <git@github.com:adunstan/postgresql-dev.git> > > Andrew, can't you get your own repo at git.postgresql.org? He certainly could, but github provides more features and a nicer look :-) And since it's git, it doesn't matter where the repo is. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Excerpts from Magnus Hagander's message of mar oct 19 05:23:31 -0300 2010: > > He certainly could, but github provides more features and a nicer look > :-) And since it's git, it doesn't matter where the repo is. Yeah. If you have a checked out copy of the GIT repo (preferably one with the "master" branch in it), try this: git remote add venum git://github.com/adunstan/postgresql-dev.git git fetch venum venums:venums git checkout venums Then you have the patch in all its Git glory, in branch "venums". -- Álvaro Herrera <alvherre@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
On 10/19/2010 12:21 AM, Andrew Dunstan wrote: > > > On 10/18/2010 10:52 AM, Tom Lane wrote: >> We could possibly deal with enum types that follow the existing >> convention if we made the cache entry hold a list of all the original, >> known-to-be-sorted OIDs. (This could be reasonably compact and cheap to >> probe if it were represented as a starting OID and a Bitmapset of delta >> values, since we can assume that the initial set of OIDs is pretty close >> together.) But we have to have that cache entry, and we have to consult >> it on every single comparison, so it's definitely going to be slower >> than before. >> >> So I'm thinking the comparison procedure goes like this: >> >> 1. Both OIDs even? >> If so, just compare them numerically, and we're done. >> >> 2. Lookup cache entry for enum type. >> >> 3. Both OIDs in list of known-sorted OIDs? >> If so, just compare them numerically, and we're done. >> >> 4. Search the part of the cache entry that lists sort positions. >> If not both present, refresh the cache entry. >> If still not present, throw error. >> >> 5. Compare by sort positions. >> >> Step 4 is the slowest part but would be avoided in most cases. >> However, step 2 is none too speedy either, and would usually >> be required when dealing with pre-existing enums. > > OK, I've made adjustments that I think do what you're suggesting. > > I've discovered and fixed a couple more bugs in this. I have one or two more things to fix and then I'll send a new patch. Meanwhile, I've been testing a database that was upgraded from 9.0, so it has a lot of odd-numbered Oids. It's not really clear from performance testing that the bitmap is a huge win, or even a win at all. (Of course, my implementation might suck too.) I'll continue testing. cheers andrew
On 10/19/2010 12:53 PM, Andrew Dunstan wrote:
>
>
> On 10/19/2010 12:21 AM, Andrew Dunstan wrote:
>>
>>
>> On 10/18/2010 10:52 AM, Tom Lane wrote:
>>> We could possibly deal with enum types that follow the existing
>>> convention if we made the cache entry hold a list of all the original,
>>> known-to-be-sorted OIDs. (This could be reasonably compact and
>>> cheap to
>>> probe if it were represented as a starting OID and a Bitmapset of delta
>>> values, since we can assume that the initial set of OIDs is pretty
>>> close
>>> together.) But we have to have that cache entry, and we have to
>>> consult
>>> it on every single comparison, so it's definitely going to be slower
>>> than before.
>>>
>>> So I'm thinking the comparison procedure goes like this:
>>>
>>> 1. Both OIDs even?
>>> If so, just compare them numerically, and we're done.
>>>
>>> 2. Lookup cache entry for enum type.
>>>
>>> 3. Both OIDs in list of known-sorted OIDs?
>>> If so, just compare them numerically, and we're done.
>>>
>>> 4. Search the part of the cache entry that lists sort positions.
>>> If not both present, refresh the cache entry.
>>> If still not present, throw error.
>>>
>>> 5. Compare by sort positions.
>>>
>>> Step 4 is the slowest part but would be avoided in most cases.
>>> However, step 2 is none too speedy either, and would usually
>>> be required when dealing with pre-existing enums.
>>
>> OK, I've made adjustments that I think do what you're suggesting.
>>
>>
>
> I've discovered and fixed a couple more bugs in this. I have one or
> two more things to fix and then I'll send a new patch.
>
> Meanwhile, I've been testing a database that was upgraded from 9.0,
> so it has a lot of odd-numbered Oids. It's not really clear from
> performance testing that the bitmap is a huge win, or even a win at
> all. (Of course, my implementation might suck too.) I'll continue
> testing.
>
>
Well a bit more testing shows some benefit. I've sorted out a few kinks,
so this seems to work. In particular, with the above tables, the version
imported from 9.0 can create have an index created in about the same
time as on the fresh table (identical data, but all even numbered Oids).
Of course, with lots of odd numbered Oids, if a label gets added the
imported version will degrade in performance much more quickly.
The test timed is:
do $$ begin for i in 1 .. 20 loop drop index if exists idx1;
create index idx1 on mydata(label); end loop; end; $$;
Latest patch attached.
cheers
andrew
Attachment
On Tue, Oct 19, 2010 at 5:42 PM, Andrew Dunstan <andrew@dunslane.net> wrote: > Well a bit more testing shows some benefit. I've sorted out a few kinks, so > this seems to work. In particular, with the above tables, the version > imported from 9.0 can create have an index created in about the same time as > on the fresh table (identical data, but all even numbered Oids). > > Of course, with lots of odd numbered Oids, if a label gets added the > imported version will degrade in performance much more quickly. I'm quite impressed by the amount of time and thought being put into optimizing this. I didn't realize people cared so much about enum performance; but it's good that they do. I hope to see more such efforts in other parts of the system. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 10/19/2010 08:51 PM, Robert Haas wrote: > On Tue, Oct 19, 2010 at 5:42 PM, Andrew Dunstan<andrew@dunslane.net> wrote: >> Well a bit more testing shows some benefit. I've sorted out a few kinks, so >> this seems to work. In particular, with the above tables, the version >> imported from 9.0 can create have an index created in about the same time as >> on the fresh table (identical data, but all even numbered Oids). >> >> Of course, with lots of odd numbered Oids, if a label gets added the >> imported version will degrade in performance much more quickly. > I'm quite impressed by the amount of time and thought being put into > optimizing this. I didn't realize people cared so much about enum > performance; but it's good that they do. > > I hope to see more such efforts in other parts of the system. :-) Efficiency has always been one of the major reasons for using enums, so it's important that we make them extensible without badly affecting performance. cheers andrew
On Tue, Oct 19, 2010 at 08:51:16PM -0400, Robert Haas wrote: > On Tue, Oct 19, 2010 at 5:42 PM, Andrew Dunstan <andrew@dunslane.net> wrote: > > Well a bit more testing shows some benefit. I've sorted out a few kinks, so > > this seems to work. In particular, with the above tables, the version > > imported from 9.0 can create have an index created in about the same time as > > on the fresh table (identical data, but all even numbered Oids). > > > > Of course, with lots of odd numbered Oids, if a label gets added the > > imported version will degrade in performance much more quickly. > > I'm quite impressed by the amount of time and thought being put into > optimizing this. I didn't realize people cared so much about enum > performance; but it's good that they do. > > I hope to see more such efforts in other parts of the system. Which parts of the system, in particular, do you have in mind? Other people from EDB have mentioned that slimming down the on-disk representation was one such target. What other ones would you see as needing such attention? Cheers, David. -- David Fetter <david@fetter.org> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fetter@gmail.com iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
On Wed, Oct 20, 2010 at 3:16 PM, David Fetter <david@fetter.org> wrote: > On Tue, Oct 19, 2010 at 08:51:16PM -0400, Robert Haas wrote: >> On Tue, Oct 19, 2010 at 5:42 PM, Andrew Dunstan <andrew@dunslane.net> wrote: >> > Well a bit more testing shows some benefit. I've sorted out a few kinks, so >> > this seems to work. In particular, with the above tables, the version >> > imported from 9.0 can create have an index created in about the same time as >> > on the fresh table (identical data, but all even numbered Oids). >> > >> > Of course, with lots of odd numbered Oids, if a label gets added the >> > imported version will degrade in performance much more quickly. >> >> I'm quite impressed by the amount of time and thought being put into >> optimizing this. I didn't realize people cared so much about enum >> performance; but it's good that they do. >> >> I hope to see more such efforts in other parts of the system. > > Which parts of the system, in particular, do you have in mind? Other > people from EDB have mentioned that slimming down the on-disk > representation was one such target. What other ones would you see as > needing such attention? On-disk footprint. WAL volume. COPY speed. Checkpoint I/O. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Tue, Oct 19, 2010 at 9:15 PM, Andrew Dunstan <andrew@dunslane.net> wrote: > Efficiency has always been one of the major reasons for using enums, so > it's important that we make them extensible without badly affecting > performance. on that note is it worthwhile backpatching recent versions to allocate enums with even numbered oids? That way people binary upgrading can get the benefit of the optimization they should qualify for... merlin
On Wed, Oct 20, 2010 at 6:54 PM, Merlin Moncure <mmoncure@gmail.com> wrote: > On Tue, Oct 19, 2010 at 9:15 PM, Andrew Dunstan <andrew@dunslane.net> wrote: >> Efficiency has always been one of the major reasons for using enums, so >> it's important that we make them extensible without badly affecting >> performance. > > on that note is it worthwhile backpatching recent versions to allocate > enums with even numbered oids? That way people binary upgrading can > get the benefit of the optimization they should qualify for... Uh, -1 from me. This is not a bug fix, and it will only help people who create new enums between the time they upgrade to the relevant minor release and the time they upgrade to 9.1. We are not into the business of back-patching marginal peformance enhancements. If we want to have a 9.0R2 release, or whatever, then so be it, but let's not be modifying behavior in stable branches unless there's a *bug*. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Andrew Dunstan <andrew@dunslane.net> writes:
> Latest patch attached.
I've been working through this patch. It occurs to me that there's a
fairly serious problem with the current implementation of insertion of
new values within the bounds of the current sort ordering. Namely, that
it does that by reassigning the enumsortorder values of pre-existing
rows. That creates a race condition: suppose that our backend is doing
that while another process is busy loading its internal cache of values
of the enum. If we commit partway through the other process's loading
of its cache, it may end up with a cache containing some pre-commit
entries and some post-commit entries. In the worst case it might even
have two images of the same enum label, with different enumsortorder
values. Needless to say, this is catastrophic for correctness of
subsequent comparisons in the other process.
We could try to avoid the race condition, but it's not going to be easy.
I think a better idea is to avoid having to issue updates against
pg_enum rows once they are inserted. To do that, I propose that instead
of integer enumsortorder values, we use float8 values. The initial
entries for a type would still have numbers 1..n, but when we need to
insert a value between two existing entries, we assign it a value
halfway between their enumsortorder values. Then we never have to alter
pre-existing entries, and there's no race condition: at worst, a
process's cache entry might be missing some just-committed rows, and we
know how to deal with that.
The disadvantage of this scheme is that if you repeatedly insert entries
in the "same place" in the sort order, you halve the available range
each time, so you'd run out of room after order-of-fifty halvings.
The values would eventually differ by only one unit in the last place,
so it'd not be possible to insert another value that would still be
distinguishable in the sort order. Is that an acceptable restriction?
(If you did run into this, you could manually reassign enumsortorder
values to get out of it, without having to dump-and-reload; but you'd
have to beware of the same race condition as above.) Of course adding
new values at the start or end of the enum's list wouldn't have that
restriction.
Thoughts?
regards, tom lane
> The disadvantage of this scheme is that if you repeatedly insert entries
> in the "same place" in the sort order, you halve the available range
> each time, so you'd run out of room after order-of-fifty halvings.
This is not a real issue. If anyone is using an ENUM with 1000 values
in it, they're doing it wrong. However, we'd have to present an
intelligible error message in that case.
The float method would also have a couple other issues:
(1) larger value for the enum order, so more RAM. Do we care?
(2) would need to create a view which hid the floats from admins who
just want to look at the enum ordering.
-- -- Josh Berkus PostgreSQL Experts Inc.
http://www.pgexperts.com
Josh Berkus <josh@agliodbs.com> writes:
>> The disadvantage of this scheme is that if you repeatedly insert entries
>> in the "same place" in the sort order, you halve the available range
>> each time, so you'd run out of room after order-of-fifty halvings.
> This is not a real issue. If anyone is using an ENUM with 1000 values
> in it, they're doing it wrong. However, we'd have to present an
> intelligible error message in that case.
You wouldn't need 1000 values to get into trouble. If you did
CREATE TYPE e AS ENUM ('a', 'b');
ALTER TYPE e ADD 'c1' BEFORE 'b';ALTER TYPE e ADD 'c2' BEFORE 'b';ALTER TYPE e ADD 'c3' BEFORE 'b';ALTER TYPE e ADD
'c4'BEFORE 'b';...
you'd hit the failure somewhere around c50, assuming IEEE-style floats.
I think an "intelligible error message" wouldn't be hard; it'd say
something like "no more room to insert another enum label between enum
labels c49 and b".
> The float method would also have a couple other issues:
> (1) larger value for the enum order, so more RAM. Do we care?
The in-memory representation wouldn't be any bigger, because we don't
actually need to keep the enumorder values in the cache entries.
pg_enum rows would get wider, but not materially so considering they
already contain a 64-byte name column.
> (2) would need to create a view which hid the floats from admins who
> just want to look at the enum ordering.
Why? We weren't going to hide the enumorder values before, why would we
do it with this?
regards, tom lane
On Oct 23, 2010, at 7:12 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Andrew Dunstan <andrew@dunslane.net> writes: >> Latest patch attached. > > I've been working through this patch. It occurs to me that there's a > fairly serious problem with the current implementation of insertion of > new values within the bounds of the current sort ordering. Namely, that > it does that by reassigning the enumsortorder values of pre-existing > rows. That creates a race condition: It strikes me that this is merely one facet of our failure to do proper locking on DDL objects other than relations, andthat this would be as good a time as any to start fixing it. ISTM that ALTER TYPE should grab a self-excluding lock justas ALTER TABLE already does. ...Robert
Robert Haas <robertmhaas@gmail.com> writes:
> On Oct 23, 2010, at 7:12 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I've been working through this patch. It occurs to me that there's a
>> fairly serious problem with the current implementation of insertion of
>> new values within the bounds of the current sort ordering. Namely, that
>> it does that by reassigning the enumsortorder values of pre-existing
>> rows. That creates a race condition:
> It strikes me that this is merely one facet of our failure to do proper locking on DDL objects other than relations,
andthat this would be as good a time as any to start fixing it. ISTM that ALTER TYPE should grab a self-excluding lock
justas ALTER TABLE already does.
The point of all the design thrashing we've been doing here is to
*avoid* taking locks while comparing enum OIDs. So I'm not impressed
with this proposal. (A self-exclusive lock to prevent concurrent
ALTER TYPEs might be a good idea, but I don't want to block enum
comparisons too.)
I did just think of a possible solution that would work with the
updating implementation: readers of pg_enum could use an MVCC snapshot
instead of SnapshotNow while loading their caches. I'm not certain
offhand how unpleasant this'd be at the C-code level, but it should
be possible. I still prefer the idea of not changing rows once they're
inserted, though --- doing so could possibly also cause transient
failures in, eg, enum_in/enum_out, since those depend on syscaches that
are loaded with SnapshotNow.
regards, tom lane
On Oct 23, 2010, at 7:52 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > I still prefer the idea of not changing rows once they're > inserted, though Me too. But I really dislike the idea of having a failure mode where we can't insert for no reason that the user can understand. So I'm trying to think of a better option. Why would you need to lock out type comparisons? Locking out concurrent DDL seems sufficient. ...Robert
Robert Haas <robertmhaas@gmail.com> writes:
> Why would you need to lock out type comparisons?
Didn't you get the point? The hazard is to a concurrent process that is
merely trying to load up its enum-values cache so that it can perform an
enum comparison. I don't want such an operation to have to block,
especially not against something that's trying to acquire a more or less
exclusive lock.
regards, tom lane
On 10/23/2010 07:12 PM, Tom Lane wrote: > Andrew Dunstan<andrew@dunslane.net> writes: >> Latest patch attached. > I've been working through this patch. Cool. [snip: reallocating enum sortorder on existing values has race conditions. Suggestion to use a float8 instead and add new value half way between neighbours, so no reassignment is necessary] > The disadvantage of this scheme is that if you repeatedly insert entries > in the "same place" in the sort order, you halve the available range > each time, so you'd run out of room after order-of-fifty halvings. > The values would eventually differ by only one unit in the last place, > so it'd not be possible to insert another value that would still be > distinguishable in the sort order. Is that an acceptable restriction? > (If you did run into this, you could manually reassign enumsortorder > values to get out of it, without having to dump-and-reload; but you'd > have to beware of the same race condition as above.) Of course adding > new values at the start or end of the enum's list wouldn't have that > restriction. Well, it's a tiny bit like my initial proposal for enum Oid ranges with gaps, but with a level of indirection :-) Seriously, I think it might be OK. Could we provide some safe way of resetting the sortorder values? Or even a not-entirely-safe superuser-only function might be useful. Binary upgrade could probably call it safely, for example. We've sure expended quite a lot of neurons on this feature :-) cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> Seriously, I think it might be OK. Could we provide some safe way of
> resetting the sortorder values? Or even a not-entirely-safe
> superuser-only function might be useful. Binary upgrade could probably
> call it safely, for example.
You could do it with plain SQL, as long as you weren't concerned about
confusing processes that were concurrently loading their enum caches.
Another thought here is that the split-in-half rule might be
unnecessarily dumb. It leaves equal amounts of code space on both sides
of the new value, even though the odds of subsequent insertions on both
sides are probably unequal. But I'm not sure if we can predict the
usage pattern well enough to know which side is more likely.
regards, tom lane
On Sat, Oct 23, 2010 at 8:11 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> Why would you need to lock out type comparisons? > > Didn't you get the point? The hazard is to a concurrent process that is > merely trying to load up its enum-values cache so that it can perform an > enum comparison. I don't want such an operation to have to block, > especially not against something that's trying to acquire a more or less > exclusive lock. Hmm, yeah, I missed the point. Sorry. I suppose you could fix this by always updating every row, and storing in each row the total count of elements (or a random number). Then it'd be obvious if you'd read an inconsistent view of the world. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 10/23/2010 08:54 PM, Tom Lane wrote: > Another thought here is that the split-in-half rule might be > unnecessarily dumb. It leaves equal amounts of code space on both sides > of the new value, even though the odds of subsequent insertions on both > sides are probably unequal. But I'm not sure if we can predict the > usage pattern well enough to know which side is more likely. We can't. In particular, we can't rely on the label to tell us, so we have no information at all to go on, really. Let's just go with the simple midpoint. Are you going to try doing this? cheers andrew
Robert Haas <robertmhaas@gmail.com> writes:
> I suppose you could fix this by always updating every row, and storing
> in each row the total count of elements (or a random number). Then
> it'd be obvious if you'd read an inconsistent view of the world.
Well, the easy way to read a consistent view of the world is to load the
cache using an MVCC snapshot instead of SnapshotNow. The current code
structure isn't amenable to that because it's relying on a syscache to
fetch the data for it, but that seems pretty inefficient anyway. I'm
thinking of changing it around so that the enum cache gets loaded with
a regular systable_beginscan() scan, and then we could load with an
MVCC snapshot.
I'm kind of inclined to go to the float-based representation anyway,
though, just because not having to update other rows to do an insert
seems like a good thing. But we could combine that with an MVCC
snapshot on the read side, which would make renumbering safe, which
would mean that we could auto-renumber when we ran out of code space
and not otherwise. Is that getting too complicated?
regards, tom lane
On Sun, Oct 24, 2010 at 12:26 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> I suppose you could fix this by always updating every row, and storing >> in each row the total count of elements (or a random number). Then >> it'd be obvious if you'd read an inconsistent view of the world. > > Well, the easy way to read a consistent view of the world is to load the > cache using an MVCC snapshot instead of SnapshotNow. Yeah, I have to admit I had never given much thought to how evil SnapshotNow is. I wonder if we have latent bugs because of this. Seems like anything where an object is represented by more than one table row is suspect. > The current code > structure isn't amenable to that because it's relying on a syscache to > fetch the data for it, but that seems pretty inefficient anyway. I'm > thinking of changing it around so that the enum cache gets loaded with > a regular systable_beginscan() scan, and then we could load with an > MVCC snapshot. That sounds reasonable. > I'm kind of inclined to go to the float-based representation anyway, > though, just because not having to update other rows to do an insert > seems like a good thing. But we could combine that with an MVCC > snapshot on the read side, which would make renumbering safe, which > would mean that we could auto-renumber when we ran out of code space > and not otherwise. Is that getting too complicated? The complexity doesn't bother me, but I'd probably just renumber every time. It's worth optimizing enums for speed, but micro-optimizing the enum DDL for speed may not be worth it, especially because it will ensure that the renumbering code is very rarely tested. If you are going to only renumber when necessary, I'd probably make the ordering position an integer rather than a float, and just set the values for the initial labels to something like 256 * 10^16, 257 * 10^16, 258 * 10^16, 259 * 10^16; and then some regression tests that add labels a, z, y, x, .... I dunno if floats have completely consistent representations on all the platforms we support, but with integers it should be quite easy to predict the exact point when you're going to run out of space and what the contents of pg_enum should be just before and just after that point. -- Robert Haas President, Floating-Point Haters Anonymous
On Sat, Oct 23, 2010 at 10:11 PM, Robert Haas <robertmhaas@gmail.com> wrote: > I dunno if floats have completely > consistent representations on all the platforms we support, but with > integers it should be quite easy to predict the exact point when > you're going to run out of space and what the contents of pg_enum > should be just before and just after that point. There's nothing magic about the integral types here. If you use a string then you could always split by making the string longer. It would get weird after you get to 256 (you can still handle it but there would be weird special case code) but an array of integers would be just as flexible and wouldn't have the same problem. -- greg
On 24 October 2010 05:26, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Well, the easy way to read a consistent view of the world is to load the > cache using an MVCC snapshot instead of SnapshotNow. The current code > structure isn't amenable to that because it's relying on a syscache to > fetch the data for it, but that seems pretty inefficient anyway. I'm > thinking of changing it around so that the enum cache gets loaded with > a regular systable_beginscan() scan, and then we could load with an > MVCC snapshot. > > I'm kind of inclined to go to the float-based representation anyway, > though, just because not having to update other rows to do an insert > seems like a good thing. But we could combine that with an MVCC > snapshot on the read side, which would make renumbering safe, which > would mean that we could auto-renumber when we ran out of code space > and not otherwise. Is that getting too complicated? > As an alternative, how about storing the sort order as an array of OIDs, in a single row in pg_type, or a new table, rather than across multiple rows in pg_enum. Code that read it would be guaranteed a consistent view of the data, and at worst, it might be missing recently added elements, which the comparison code can already deal with by re-reading the array. Regards, Dean > regards, tom lane >
Greg Stark <gsstark@mit.edu> writes:
> There's nothing magic about the integral types here. If you use a
> string then you could always split by making the string longer.
The entire objective here is to make enum comparisons fast. Somehow,
going to a non-primitive data type to represent the sort values does
not seem like a win from that perspective.
(Likewise for Dean's suggestion for an array of another kind.)
What I'm currently thinking is that we should actually use float4 not
float8. That eliminates any concerns about making the representation
larger than it was before. We'll definitely have to have the logic
to do renumbering, but it'll still be an exceedingly rare thing in
practice. With float4 the implementation would fail at somewhere
around 2^24 elements in an enum (since even with renumbering, there
wouldn't be enough bits to give each element a distinguishable value).
I don't see that as a real objection, and anyway if you were trying
to have an enum with many elements, you'd want the in-memory
representation to be compact.
regards, tom lane
On 10/24/2010 12:20 PM, Tom Lane wrote: > With float4 the implementation would fail at somewhere > around 2^24 elements in an enum (since even with renumbering, there > wouldn't be enough bits to give each element a distinguishable value). > I don't see that as a real objection, and anyway if you were trying > to have an enum with many elements, you'd want the in-memory > representation to be compact. Anything beyond the square root of this is getting pretty insane, IMNSHO, so I'm really not that bothered by that number. Assuming we renumber the sortorder as even positive integers, that number comes down a couple of bits, but even so that gives us lots of head room I think. cheers andrew
On 24 October 2010 17:20, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Greg Stark <gsstark@mit.edu> writes:
>> There's nothing magic about the integral types here. If you use a
>> string then you could always split by making the string longer.
>
> The entire objective here is to make enum comparisons fast. Somehow,
> going to a non-primitive data type to represent the sort values does
> not seem like a win from that perspective.
>
> (Likewise for Dean's suggestion for an array of another kind.)
>
The point with an OID array is that you wouldn't need to store the
enumsortorder values at all. The sort order would just be the index of
the OID in the array. So the comparison code would read the OID array,
traverse it building an array of enum_sort structs {oid, idx}, sort
that by OID and cache it. Then do binary searches to efficiently find
the index (i.e., sort order) for any given OID. That's pretty much
what the comparison code is doing now, except without explicitly
stored sort positions.
The reason I thought this might help is that it would be a single
atomic operation to read the whole enum sort order, so you'd never get
a mix of old and new sort positions, if another transaction was
altering the enum.
> What I'm currently thinking is that we should actually use float4 not
> float8. That eliminates any concerns about making the representation
> larger than it was before. We'll definitely have to have the logic
> to do renumbering, but it'll still be an exceedingly rare thing in
> practice. With float4 the implementation would fail at somewhere
> around 2^24 elements in an enum (since even with renumbering, there
> wouldn't be enough bits to give each element a distinguishable value).
> I don't see that as a real objection, and anyway if you were trying
> to have an enum with many elements, you'd want the in-memory
> representation to be compact.
>
That seems like a lot of additional complexity to do the renumbering,
and you'd have to make the comparison code safe against a concurrent
renumbering.
Regards,
Dean
> regards, tom lane
>
Andrew Dunstan <andrew@dunslane.net> writes:
> On 10/24/2010 12:20 PM, Tom Lane wrote:
>> With float4 the implementation would fail at somewhere
>> around 2^24 elements in an enum (since even with renumbering, there
>> wouldn't be enough bits to give each element a distinguishable value).
>> I don't see that as a real objection, and anyway if you were trying
>> to have an enum with many elements, you'd want the in-memory
>> representation to be compact.
> Anything beyond the square root of this is getting pretty insane,
> IMNSHO, so I'm really not that bothered by that number.
Here's a WIP patch that incorporates most of what's been discussed here.
The critical part of it is summed up in the comments for RenumberEnumType:
/*
* RenumberEnumType
* Renumber existing enum elements to have sort positions 1..n.
*
* We avoid doing this unless absolutely necessary; in most installations
* it will never happen. The reason is that updating existing pg_enum
* entries creates hazards for other backends that are concurrently reading
* pg_enum with SnapshotNow semantics. A concurrent SnapshotNow scan could
* see both old and new versions of an updated row as valid, or neither of
* them, if the commit happens between scanning the two versions. It's
* also quite likely for a concurrent scan to see an inconsistent set of
* rows (some members updated, some not).
*
* We can avoid these risks by reading pg_enum with an MVCC snapshot
* instead of SnapshotNow, but that forecloses use of the syscaches.
* We therefore make the following choices:
*
* 1. Any code that is interested in the enumsortorder values MUST read
* pg_enum with an MVCC snapshot, or else acquire lock on the enum type
* to prevent concurrent execution of AddEnumLabel(). The risk of
* seeing inconsistent values of enumsortorder is too high otherwise.
*
* 2. Code that is not examining enumsortorder can use a syscache
* (for example, enum_in and enum_out do so). The worst that can happen
* is a transient failure to find any valid value of the row. This is
* judged acceptable in view of the infrequency of use of RenumberEnumType.
*/
This patch isn't committable as-is because I haven't made enum_first,
enum_last, enum_range follow these coding rules: they need to stop
using the syscache and instead use an indexscan on
pg_enum_typid_sortorder_index to locate the relevant rows. That should
be just a small fix though, and it seems likely to be a net win for
performance anyway. There are a couple of other loose ends
too, in particular I still think we need to prevent ALTER TYPE ADD
inside a transaction block because of the risk of finding undefined
enum OIDs in indexes.
Anybody really unhappy with this approach? If not, I'll finish this
up and commit it.
regards, tom lane
diff --git a/contrib/pg_upgrade/function.c b/contrib/pg_upgrade/function.c
index 31255d637dc8a6fd2642650bccfec972401f188f..c76aaeb090bde61c4308807033164946f2aa22cc 100644
*** a/contrib/pg_upgrade/function.c
--- b/contrib/pg_upgrade/function.c
*************** install_support_functions(void)
*** 79,85 ****
"LANGUAGE C STRICT;"));
PQclear(executeQueryOrDie(conn,
"CREATE OR REPLACE FUNCTION "
! " binary_upgrade.add_pg_enum_label(OID, OID, NAME) "
"RETURNS VOID "
"AS '$libdir/pg_upgrade_support' "
"LANGUAGE C STRICT;"));
--- 79,85 ----
"LANGUAGE C STRICT;"));
PQclear(executeQueryOrDie(conn,
"CREATE OR REPLACE FUNCTION "
! " binary_upgrade.set_next_pg_enum_oid(OID) "
"RETURNS VOID "
"AS '$libdir/pg_upgrade_support' "
"LANGUAGE C STRICT;"));
diff --git a/contrib/pg_upgrade_support/pg_upgrade_support.c b/contrib/pg_upgrade_support/pg_upgrade_support.c
index c956be187ac398495944d3f0b8c07ec18a970040..3ec436fe140c2effcc5793e2c39cb6d6949dc975 100644
*** a/contrib/pg_upgrade_support/pg_upgrade_support.c
--- b/contrib/pg_upgrade_support/pg_upgrade_support.c
***************
*** 16,28 ****
/* THIS IS USED ONLY FOR PG >= 9.0 */
- /*
- * Cannot include "catalog/pg_enum.h" here because we might
- * not be compiling against PG 9.0.
- */
- extern void EnumValuesCreate(Oid enumTypeOid, List *vals,
- Oid binary_upgrade_next_pg_enum_oid);
-
#ifdef PG_MODULE_MAGIC
PG_MODULE_MAGIC;
#endif
--- 16,21 ----
*************** extern PGDLLIMPORT Oid binary_upgrade_ne
*** 33,38 ****
--- 26,32 ----
extern PGDLLIMPORT Oid binary_upgrade_next_heap_relfilenode;
extern PGDLLIMPORT Oid binary_upgrade_next_toast_relfilenode;
extern PGDLLIMPORT Oid binary_upgrade_next_index_relfilenode;
+ extern PGDLLIMPORT Oid binary_upgrade_next_pg_enum_oid;
Datum set_next_pg_type_oid(PG_FUNCTION_ARGS);
Datum set_next_pg_type_array_oid(PG_FUNCTION_ARGS);
*************** Datum set_next_pg_type_toast_oid(PG_FUN
*** 40,46 ****
Datum set_next_heap_relfilenode(PG_FUNCTION_ARGS);
Datum set_next_toast_relfilenode(PG_FUNCTION_ARGS);
Datum set_next_index_relfilenode(PG_FUNCTION_ARGS);
! Datum add_pg_enum_label(PG_FUNCTION_ARGS);
PG_FUNCTION_INFO_V1(set_next_pg_type_oid);
PG_FUNCTION_INFO_V1(set_next_pg_type_array_oid);
--- 34,40 ----
Datum set_next_heap_relfilenode(PG_FUNCTION_ARGS);
Datum set_next_toast_relfilenode(PG_FUNCTION_ARGS);
Datum set_next_index_relfilenode(PG_FUNCTION_ARGS);
! Datum set_next_pg_enum_oid(PG_FUNCTION_ARGS);
PG_FUNCTION_INFO_V1(set_next_pg_type_oid);
PG_FUNCTION_INFO_V1(set_next_pg_type_array_oid);
*************** PG_FUNCTION_INFO_V1(set_next_pg_type_toa
*** 48,54 ****
PG_FUNCTION_INFO_V1(set_next_heap_relfilenode);
PG_FUNCTION_INFO_V1(set_next_toast_relfilenode);
PG_FUNCTION_INFO_V1(set_next_index_relfilenode);
! PG_FUNCTION_INFO_V1(add_pg_enum_label);
Datum
set_next_pg_type_oid(PG_FUNCTION_ARGS)
--- 42,48 ----
PG_FUNCTION_INFO_V1(set_next_heap_relfilenode);
PG_FUNCTION_INFO_V1(set_next_toast_relfilenode);
PG_FUNCTION_INFO_V1(set_next_index_relfilenode);
! PG_FUNCTION_INFO_V1(set_next_pg_enum_oid);
Datum
set_next_pg_type_oid(PG_FUNCTION_ARGS)
*************** set_next_index_relfilenode(PG_FUNCTION_A
*** 111,124 ****
}
Datum
! add_pg_enum_label(PG_FUNCTION_ARGS)
{
Oid enumoid = PG_GETARG_OID(0);
- Oid typoid = PG_GETARG_OID(1);
- Name label = PG_GETARG_NAME(2);
! EnumValuesCreate(typoid, list_make1(makeString(NameStr(*label))),
! enumoid);
PG_RETURN_VOID();
}
--- 105,115 ----
}
Datum
! set_next_pg_enum_oid(PG_FUNCTION_ARGS)
{
Oid enumoid = PG_GETARG_OID(0);
! binary_upgrade_next_pg_enum_oid = enumoid;
PG_RETURN_VOID();
}
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index b7b48e4fb93c26c421cea5c6fd01a914f9ccc8b5..9a8729b8b3194a780ea4b15dd5cfcd8bff297543 100644
*** a/doc/src/sgml/catalogs.sgml
--- b/doc/src/sgml/catalogs.sgml
***************
*** 2623,2634 ****
<para>
The <structname>pg_enum</structname> catalog contains entries
! matching enum types to their associated values and labels. The
internal representation of a given enum value is actually the OID
! of its associated row in <structname>pg_enum</structname>. The
! OIDs for a particular enum type are guaranteed to be ordered in
! the way the type should sort, but there is no guarantee about the
! ordering of OIDs of unrelated enum types.
</para>
<table>
--- 2623,2631 ----
<para>
The <structname>pg_enum</structname> catalog contains entries
! showing the values and labels for each enum type. The
internal representation of a given enum value is actually the OID
! of its associated row in <structname>pg_enum</structname>.
</para>
<table>
***************
*** 2653,2658 ****
--- 2650,2662 ----
</row>
<row>
+ <entry><structfield>enumsortorder</structfield></entry>
+ <entry><type>float4</type></entry>
+ <entry></entry>
+ <entry>The sort position of this enum value within its enum type</entry>
+ </row>
+
+ <row>
<entry><structfield>enumlabel</structfield></entry>
<entry><type>name</type></entry>
<entry></entry>
***************
*** 2661,2666 ****
--- 2665,2690 ----
</tbody>
</tgroup>
</table>
+
+ <para>
+ The OIDs for <structname>pg_enum</structname> rows follow a special
+ rule: even-numbered OIDs are guaranteed to be ordered in the same way
+ as the sort ordering of their enum type. That is, if two even OIDs
+ belong to the same enum type, the smaller OID must have the smaller
+ <structfield>enumsortorder</structfield> value. Odd-numbered OID values
+ need bear no relationship to the sort order. This rule allows the
+ enum comparison routines to avoid catalog lookups in many common cases.
+ The routines that create and alter enum types attempt to assign even
+ OIDs to enum values whenever possible.
+ </para>
+
+ <para>
+ When an enum type is created, its members are assigned sort-order
+ positions 1..<replaceable>n</>. But members added later might be given
+ negative or fractional values of <structfield>enumsortorder</structfield>.
+ The only requirement on these values is that they be correctly
+ ordered and unique within each enum type.
+ </para>
</sect1>
diff --git a/doc/src/sgml/ref/alter_type.sgml b/doc/src/sgml/ref/alter_type.sgml
index 315922ea8365c482cdd787d1a775e0220ba1625c..b1336e1d0dcd38a2ca572495150b441d192b2fe7 100644
*** a/doc/src/sgml/ref/alter_type.sgml
--- b/doc/src/sgml/ref/alter_type.sgml
*************** PostgreSQL documentation
*** 24,33 ****
<refsynopsisdiv>
<synopsis>
ALTER TYPE <replaceable class="PARAMETER">name</replaceable> <replaceable class="PARAMETER">action</replaceable> [,
...]
! ALTER TYPE <replaceable class="PARAMETER">name</replaceable> OWNER TO <replaceable
class="PARAMETER">new_owner</replaceable>
ALTER TYPE <replaceable class="PARAMETER">name</replaceable> RENAME ATTRIBUTE <replaceable
class="PARAMETER">attribute_name</replaceable>TO <replaceable class="PARAMETER">new_attribute_name</replaceable>
ALTER TYPE <replaceable class="PARAMETER">name</replaceable> RENAME TO <replaceable
class="PARAMETER">new_name</replaceable>
ALTER TYPE <replaceable class="PARAMETER">name</replaceable> SET SCHEMA <replaceable
class="PARAMETER">new_schema</replaceable>
<phrase>where <replaceable class="PARAMETER">action</replaceable> is one of:</phrase>
--- 24,34 ----
<refsynopsisdiv>
<synopsis>
ALTER TYPE <replaceable class="PARAMETER">name</replaceable> <replaceable class="PARAMETER">action</replaceable> [,
...]
! ALTER TYPE <replaceable class="PARAMETER">name</replaceable> OWNER TO <replaceable
class="PARAMETER">new_owner</replaceable>
ALTER TYPE <replaceable class="PARAMETER">name</replaceable> RENAME ATTRIBUTE <replaceable
class="PARAMETER">attribute_name</replaceable>TO <replaceable class="PARAMETER">new_attribute_name</replaceable>
ALTER TYPE <replaceable class="PARAMETER">name</replaceable> RENAME TO <replaceable
class="PARAMETER">new_name</replaceable>
ALTER TYPE <replaceable class="PARAMETER">name</replaceable> SET SCHEMA <replaceable
class="PARAMETER">new_schema</replaceable>
+ ALTER TYPE <replaceable class="PARAMETER">name</replaceable> ADD <replaceable
class="PARAMETER">new_enum_value</replaceable>[ { BEFORE | AFTER } <replaceable
class="PARAMETER">existing_enum_value</replaceable>]
<phrase>where <replaceable class="PARAMETER">action</replaceable> is one of:</phrase>
*************** ALTER TYPE <replaceable class="PARAMETER
*** 103,108 ****
--- 104,121 ----
</para>
</listitem>
</varlistentry>
+
+ <varlistentry>
+ <term><literal>ADD [ BEFORE | AFTER ]</literal></term>
+ <listitem>
+ <para>
+ This form adds a new value to an enum type. If the new value's place in
+ the enum's ordering is not specified using <literal>BEFORE</literal> or
+ <literal>AFTER</literal>, then the new item is placed at the end of the
+ list of values.
+ </para>
+ </listitem>
+ </varlistentry>
</variablelist>
</para>
*************** ALTER TYPE <replaceable class="PARAMETER
*** 181,187 ****
<term><replaceable class="PARAMETER">new_attribute_name</replaceable></term>
<listitem>
<para>
! The new name of the attribute begin renamed.
</para>
</listitem>
</varlistentry>
--- 194,200 ----
<term><replaceable class="PARAMETER">new_attribute_name</replaceable></term>
<listitem>
<para>
! The new name of the attribute to be renamed.
</para>
</listitem>
</varlistentry>
*************** ALTER TYPE <replaceable class="PARAMETER
*** 196,206 ****
--- 209,253 ----
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><replaceable class="PARAMETER">new_enum_value</replaceable></term>
+ <listitem>
+ <para>
+ The new value to be added to an enum type's list of values.
+ Like all enum literals, it needs to be quoted.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term><replaceable class="PARAMETER">existing_enum_value</replaceable></term>
+ <listitem>
+ <para>
+ The existing enum value that the new value should be added immediately
+ before or after in the enum type's sort ordering.
+ Like all enum literals, it needs to be quoted.
+ </para>
+ </listitem>
+ </varlistentry>
+
</variablelist>
</para>
</refsect1>
<refsect1>
+ <title>Notes</title>
+
+ <para>
+ Adding a new enum value will in some cases slow down comparison and
+ sorting of the enum type. This will usually only occur if
+ <literal>BEFORE</literal> or <literal>AFTER</literal> is used to set
+ the new value's sort position somewhere other than at the end
+ of the list. Optimal performance will be restored if the database is
+ dumped and reloaded, or the enum type is dropped and recreated.
+ </para>
+ </refsect1>
+
+ <refsect1>
<title>Examples</title>
<para>
*************** ALTER TYPE email SET SCHEMA customers;
*** 232,237 ****
--- 279,291 ----
ALTER TYPE compfoo ADD ATTRIBUTE f3 int;
</programlisting>
</para>
+
+ <para>
+ To add a new value to an enum type in a particular sort position:
+ <programlisting>
+ ALTER TYPE colors ADD 'orange' AFTER 'red';
+ </programlisting>
+ </para>
</refsect1>
<refsect1>
diff --git a/src/backend/catalog/pg_enum.c b/src/backend/catalog/pg_enum.c
index d544c1f4773d322b88d5c07bb192d6f136629829..0c384def7b60e17a0c0dc03bce0fc83d23d9389e 100644
*** a/src/backend/catalog/pg_enum.c
--- b/src/backend/catalog/pg_enum.c
***************
*** 15,29 ****
--- 15,38 ----
#include "access/genam.h"
#include "access/heapam.h"
+ #include "access/xact.h"
#include "catalog/catalog.h"
#include "catalog/indexing.h"
#include "catalog/pg_enum.h"
+ #include "catalog/pg_type.h"
+ #include "storage/lmgr.h"
#include "utils/builtins.h"
#include "utils/fmgroids.h"
#include "utils/rel.h"
+ #include "utils/syscache.h"
#include "utils/tqual.h"
+
+ Oid binary_upgrade_next_pg_enum_oid = InvalidOid;
+
+ static void RenumberEnumType(Relation pg_enum, HeapTuple *existing, int nelems);
static int oid_cmp(const void *p1, const void *p2);
+ static int sort_order_cmp(const void *p1, const void *p2);
/*
*************** static int oid_cmp(const void *p1, const
*** 33,43 ****
* vals is a list of Value strings.
*/
void
! EnumValuesCreate(Oid enumTypeOid, List *vals,
! Oid binary_upgrade_next_pg_enum_oid)
{
Relation pg_enum;
- TupleDesc tupDesc;
NameData enumlabel;
Oid *oids;
int elemno,
--- 42,50 ----
* vals is a list of Value strings.
*/
void
! EnumValuesCreate(Oid enumTypeOid, List *vals)
{
Relation pg_enum;
NameData enumlabel;
Oid *oids;
int elemno,
*************** EnumValuesCreate(Oid enumTypeOid, List *
*** 50,97 ****
num_elems = list_length(vals);
/*
! * XXX we do not bother to check the list of values for duplicates --- if
* you have any, you'll get a less-than-friendly unique-index violation.
! * Is it worth trying harder?
*/
pg_enum = heap_open(EnumRelationId, RowExclusiveLock);
- tupDesc = pg_enum->rd_att;
/*
! * Allocate oids
*/
oids = (Oid *) palloc(num_elems * sizeof(Oid));
! if (OidIsValid(binary_upgrade_next_pg_enum_oid))
! {
! if (num_elems != 1)
! ereport(ERROR,
! (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
! errmsg("EnumValuesCreate() can only set a single OID")));
! oids[0] = binary_upgrade_next_pg_enum_oid;
! binary_upgrade_next_pg_enum_oid = InvalidOid;
! }
! else
{
/*
! * While this method does not absolutely guarantee that we generate no
! * duplicate oids (since we haven't entered each oid into the table
! * before allocating the next), trouble could only occur if the oid
! * counter wraps all the way around before we finish. Which seems
! * unlikely.
*/
! for (elemno = 0; elemno < num_elems; elemno++)
! {
! /*
! * The pg_enum.oid is stored in user tables. This oid must be
! * preserved by binary upgrades.
! */
! oids[elemno] = GetNewOid(pg_enum);
! }
! /* sort them, just in case counter wrapped from high to low */
! qsort(oids, num_elems, sizeof(Oid), oid_cmp);
}
/* and make the entries */
memset(nulls, false, sizeof(nulls));
--- 57,98 ----
num_elems = list_length(vals);
/*
! * We do not bother to check the list of values for duplicates --- if
* you have any, you'll get a less-than-friendly unique-index violation.
! * It is probably not worth trying harder.
*/
pg_enum = heap_open(EnumRelationId, RowExclusiveLock);
/*
! * Allocate OIDs for the enum's members.
! *
! * While this method does not absolutely guarantee that we generate no
! * duplicate OIDs (since we haven't entered each oid into the table
! * before allocating the next), trouble could only occur if the OID
! * counter wraps all the way around before we finish. Which seems
! * unlikely.
*/
oids = (Oid *) palloc(num_elems * sizeof(Oid));
!
! for (elemno = 0; elemno < num_elems; elemno++)
{
/*
! * We assign even-numbered OIDs to all the new enum labels. This
! * tells the comparison functions the OIDs are in the correct sort
! * order and can be compared directly.
*/
! Oid new_oid;
!
! do {
! new_oid = GetNewOid(pg_enum);
! } while (new_oid & 1);
! oids[elemno] = new_oid;
}
+ /* sort them, just in case OID counter wrapped from high to low */
+ qsort(oids, num_elems, sizeof(Oid), oid_cmp);
+
/* and make the entries */
memset(nulls, false, sizeof(nulls));
*************** EnumValuesCreate(Oid enumTypeOid, List *
*** 112,121 ****
NAMEDATALEN - 1)));
values[Anum_pg_enum_enumtypid - 1] = ObjectIdGetDatum(enumTypeOid);
namestrcpy(&enumlabel, lab);
values[Anum_pg_enum_enumlabel - 1] = NameGetDatum(&enumlabel);
! tup = heap_form_tuple(tupDesc, values, nulls);
HeapTupleSetOid(tup, oids[elemno]);
simple_heap_insert(pg_enum, tup);
--- 113,123 ----
NAMEDATALEN - 1)));
values[Anum_pg_enum_enumtypid - 1] = ObjectIdGetDatum(enumTypeOid);
+ values[Anum_pg_enum_enumsortorder - 1] = Float4GetDatum(elemno + 1);
namestrcpy(&enumlabel, lab);
values[Anum_pg_enum_enumlabel - 1] = NameGetDatum(&enumlabel);
! tup = heap_form_tuple(RelationGetDescr(pg_enum), values, nulls);
HeapTupleSetOid(tup, oids[elemno]);
simple_heap_insert(pg_enum, tup);
*************** EnumValuesDelete(Oid enumTypeOid)
*** 164,170 ****
}
! /* qsort comparison function */
static int
oid_cmp(const void *p1, const void *p2)
{
--- 166,487 ----
}
! /*
! * AddEnumLabel
! * Add a new label to the enum set. By default it goes at
! * the end, but the user can choose to place it before or
! * after any existing set member.
! */
! void
! AddEnumLabel(Oid enumTypeOid,
! const char *newVal,
! const char *neighbor,
! bool newValIsAfter)
! {
! Relation pg_enum;
! Oid newOid;
! Datum values[Natts_pg_enum];
! bool nulls[Natts_pg_enum];
! NameData enumlabel;
! HeapTuple enum_tup;
! float4 newelemorder;
! HeapTuple *existing;
! CatCList *list;
! int nelems;
! int i;
!
! /* check length of new label is ok */
! if (strlen(newVal) > (NAMEDATALEN - 1))
! ereport(ERROR,
! (errcode(ERRCODE_INVALID_NAME),
! errmsg("invalid enum label \"%s\"", newVal),
! errdetail("Labels must be %d characters or less.",
! NAMEDATALEN - 1)));
!
! /*
! * Acquire a lock on the enum type, which we won't release until commit.
! * This ensures that two backends aren't concurrently modifying the same
! * enum type. Without that, we couldn't be sure to get a consistent
! * view of the enum members via the syscache. Note that this does not
! * block other backends from inspecting the type; see comments for
! * RenumberEnumType.
! */
! LockDatabaseObject(TypeRelationId, enumTypeOid, 0, ExclusiveLock);
!
! pg_enum = heap_open(EnumRelationId, RowExclusiveLock);
!
! /* If we have to renumber the existing members, we restart from here */
! restart:
!
! /* Get the list of existing members of the enum */
! list = SearchSysCacheList1(ENUMTYPOIDNAME,
! ObjectIdGetDatum(enumTypeOid));
! nelems = list->n_members;
!
! /* Sort the existing members by enumsortorder */
! existing = (HeapTuple *) palloc(nelems * sizeof(HeapTuple));
! for (i = 0; i < nelems; i++)
! existing[i] = &(list->members[i]->tuple);
!
! qsort(existing, nelems, sizeof(HeapTuple), sort_order_cmp);
!
! if (neighbor == NULL)
! {
! /*
! * Put the new label at the end of the list.
! * No change to existing tuples is required.
! */
! if (nelems > 0)
! {
! Form_pg_enum en = (Form_pg_enum) GETSTRUCT(existing[nelems - 1]);
!
! newelemorder = en->enumsortorder + 1;
! }
! else
! newelemorder = 1;
! }
! else
! {
! /* BEFORE or AFTER was specified */
! int nbr_index;
! int other_nbr_index;
! Form_pg_enum nbr_en;
! Form_pg_enum other_nbr_en;
!
! /* Locate the neighbor element */
! for (nbr_index = 0; nbr_index < nelems; nbr_index++)
! {
! Form_pg_enum en = (Form_pg_enum) GETSTRUCT(existing[nbr_index]);
!
! if (strcmp(NameStr(en->enumlabel), neighbor) == 0)
! break;
! }
! if (nbr_index >= nelems)
! ereport(ERROR,
! (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
! errmsg("\"%s\" is not an existing enum label",
! neighbor)));
! nbr_en = (Form_pg_enum) GETSTRUCT(existing[nbr_index]);
!
! /*
! * Attempt to assign an appropriate enumsortorder value: one less
! * than the smallest member, one more than the largest member,
! * or halfway between two existing members.
! *
! * In the "halfway" case, because of the finite precision of float4,
! * we might compute a value that's actually equal to one or the
! * other of its neighbors. In that case we renumber the existing
! * members and try again.
! */
! if (newValIsAfter)
! other_nbr_index = nbr_index + 1;
! else
! other_nbr_index = nbr_index - 1;
!
! if (other_nbr_index < 0)
! newelemorder = nbr_en->enumsortorder - 1;
! else if (other_nbr_index >= nelems)
! newelemorder = nbr_en->enumsortorder + 1;
! else
! {
! other_nbr_en = (Form_pg_enum) GETSTRUCT(existing[other_nbr_index]);
! newelemorder = (nbr_en->enumsortorder +
! other_nbr_en->enumsortorder) / 2;
! if (newelemorder == nbr_en->enumsortorder ||
! newelemorder == other_nbr_en->enumsortorder)
! {
! RenumberEnumType(pg_enum, existing, nelems);
! /* Clean up and start over */
! pfree(existing);
! ReleaseCatCacheList(list);
! goto restart;
! }
! }
! }
!
! /* Get a new OID for the new label */
! if (OidIsValid(binary_upgrade_next_pg_enum_oid))
! {
! /*
! * In binary upgrades, just add the new label with the predetermined
! * Oid. It's pg_upgrade's responsibility that the Oid meets
! * requirements.
! */
! if (neighbor != NULL)
! ereport(ERROR,
! (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
! errmsg("ALTER TYPE ADD BEFORE/AFTER is incompatible with binary upgrade")));
!
! newOid = binary_upgrade_next_pg_enum_oid;
! binary_upgrade_next_pg_enum_oid = InvalidOid;
! }
! else
! {
! /*
! * Normal case: we need to allocate a new Oid for the value.
! *
! * We want to give the new element an even-numbered Oid if it's safe,
! * which is to say it compares correctly to all pre-existing even
! * numbered Oids in the enum. Otherwise, we must give it an odd Oid.
! */
! for (;;)
! {
! bool sorts_ok;
!
! /* Get a new OID (different from all existing pg_enum tuples) */
! newOid = GetNewOid(pg_enum);
!
! /*
! * Detect whether it sorts correctly relative to existing
! * even-numbered labels of the enum. We can ignore existing
! * labels with odd Oids, since a comparison involving one of
! * those will not take the fast path anyway.
! */
! sorts_ok = true;
! for (i = 0; i < nelems; i++)
! {
! HeapTuple exists_tup = existing[i];
! Form_pg_enum exists_en = (Form_pg_enum) GETSTRUCT(exists_tup);
! Oid exists_oid = HeapTupleGetOid(exists_tup);
!
! if (exists_oid & 1)
! continue; /* ignore odd Oids */
!
! if (exists_en->enumsortorder < newelemorder)
! {
! /* should sort before */
! if (exists_oid >= newOid)
! {
! sorts_ok = false;
! break;
! }
! }
! else
! {
! /* should sort after */
! if (exists_oid <= newOid)
! {
! sorts_ok = false;
! break;
! }
! }
! }
!
! if (sorts_ok)
! {
! /* If it's even and sorts OK, we're done. */
! if ((newOid & 1) == 0)
! break;
!
! /*
! * If it's odd, and sorts OK, loop back to get another OID
! * and try again. Probably, the next available even OID
! * will sort correctly too, so it's worth trying.
! */
! }
! else
! {
! /*
! * If it's odd, and does not sort correctly, we're done.
! * (Probably, the next available even OID would sort
! * incorrectly too, so no point in trying again.)
! */
! if (newOid & 1)
! break;
!
! /*
! * If it's even, and does not sort correctly, loop back to get
! * another OID and try again. (We *must* reject this case.)
! */
! }
! }
! }
!
! /* Done with info about existing members */
! pfree(existing);
! ReleaseCatCacheList(list);
!
! /* Create the new pg_enum entry */
! memset(nulls, false, sizeof(nulls));
! values[Anum_pg_enum_enumtypid - 1] = ObjectIdGetDatum(enumTypeOid);
! values[Anum_pg_enum_enumsortorder - 1] = Float4GetDatum(newelemorder);
! namestrcpy(&enumlabel, newVal);
! values[Anum_pg_enum_enumlabel - 1] = NameGetDatum(&enumlabel);
! enum_tup = heap_form_tuple(RelationGetDescr(pg_enum), values, nulls);
! HeapTupleSetOid(enum_tup, newOid);
! simple_heap_insert(pg_enum, enum_tup);
! CatalogUpdateIndexes(pg_enum, enum_tup);
! heap_freetuple(enum_tup);
!
! heap_close(pg_enum, RowExclusiveLock);
! }
!
!
! /*
! * RenumberEnumType
! * Renumber existing enum elements to have sort positions 1..n.
! *
! * We avoid doing this unless absolutely necessary; in most installations
! * it will never happen. The reason is that updating existing pg_enum
! * entries creates hazards for other backends that are concurrently reading
! * pg_enum with SnapshotNow semantics. A concurrent SnapshotNow scan could
! * see both old and new versions of an updated row as valid, or neither of
! * them, if the commit happens between scanning the two versions. It's
! * also quite likely for a concurrent scan to see an inconsistent set of
! * rows (some members updated, some not).
! *
! * We can avoid these risks by reading pg_enum with an MVCC snapshot
! * instead of SnapshotNow, but that forecloses use of the syscaches.
! * We therefore make the following choices:
! *
! * 1. Any code that is interested in the enumsortorder values MUST read
! * pg_enum with an MVCC snapshot, or else acquire lock on the enum type
! * to prevent concurrent execution of AddEnumLabel(). The risk of
! * seeing inconsistent values of enumsortorder is too high otherwise.
! *
! * 2. Code that is not examining enumsortorder can use a syscache
! * (for example, enum_in and enum_out do so). The worst that can happen
! * is a transient failure to find any valid value of the row. This is
! * judged acceptable in view of the infrequency of use of RenumberEnumType.
! */
! static void
! RenumberEnumType(Relation pg_enum, HeapTuple *existing, int nelems)
! {
! int i;
!
! /*
! * We should only need to increase existing elements' enumsortorders,
! * never decrease them. Therefore, work from the end backwards, to avoid
! * unwanted uniqueness violations.
! */
! for (i = nelems - 1; i >= 0; i--)
! {
! HeapTuple newtup;
! Form_pg_enum en;
! float4 newsortorder;
!
! newtup = heap_copytuple(existing[i]);
! en = (Form_pg_enum) GETSTRUCT(newtup);
!
! newsortorder = i + 1;
! if (en->enumsortorder != newsortorder)
! {
! en->enumsortorder = newsortorder;
!
! simple_heap_update(pg_enum, &newtup->t_self, newtup);
!
! CatalogUpdateIndexes(pg_enum, newtup);
! }
!
! heap_freetuple(newtup);
! }
!
! /* Make the updates visible */
! CommandCounterIncrement();
! }
!
!
! /* qsort comparison function for oids */
static int
oid_cmp(const void *p1, const void *p2)
{
*************** oid_cmp(const void *p1, const void *p2)
*** 177,179 ****
--- 494,513 ----
return 1;
return 0;
}
+
+ /* qsort comparison function for tuples by sort order */
+ static int
+ sort_order_cmp(const void *p1, const void *p2)
+ {
+ HeapTuple v1 = *((const HeapTuple *) p1);
+ HeapTuple v2 = *((const HeapTuple *) p2);
+ Form_pg_enum en1 = (Form_pg_enum) GETSTRUCT(v1);
+ Form_pg_enum en2 = (Form_pg_enum) GETSTRUCT(v2);
+
+ if (en1->enumsortorder < en2->enumsortorder)
+ return -1;
+ else if (en1->enumsortorder > en2->enumsortorder)
+ return 1;
+ else
+ return 0;
+ }
diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c
index 46b156e09a30ff264e6cf2e7567a2beb8cda43cb..220be9b443b54143f4ed097a2786f40aaf66e415 100644
*** a/src/backend/commands/typecmds.c
--- b/src/backend/commands/typecmds.c
*************** static Oid findTypeTypmodinFunction(List
*** 84,90 ****
static Oid findTypeTypmodoutFunction(List *procname);
static Oid findTypeAnalyzeFunction(List *procname, Oid typeOid);
static List *get_rels_with_domain(Oid domainOid, LOCKMODE lockmode);
! static void checkDomainOwner(HeapTuple tup, TypeName *typename);
static char *domainAddConstraint(Oid domainOid, Oid domainNamespace,
Oid baseTypeOid,
int typMod, Constraint *constr,
--- 84,91 ----
static Oid findTypeTypmodoutFunction(List *procname);
static Oid findTypeAnalyzeFunction(List *procname, Oid typeOid);
static List *get_rels_with_domain(Oid domainOid, LOCKMODE lockmode);
! static void checkDomainOwner(HeapTuple tup);
! static void checkEnumOwner(HeapTuple tup);
static char *domainAddConstraint(Oid domainOid, Oid domainNamespace,
Oid baseTypeOid,
int typMod, Constraint *constr,
*************** DefineEnum(CreateEnumStmt *stmt)
*** 1150,1156 ****
false); /* Type NOT NULL */
/* Enter the enum's values into pg_enum */
! EnumValuesCreate(enumTypeOid, stmt->vals, InvalidOid);
/*
* Create the array type that goes with it.
--- 1151,1157 ----
false); /* Type NOT NULL */
/* Enter the enum's values into pg_enum */
! EnumValuesCreate(enumTypeOid, stmt->vals);
/*
* Create the array type that goes with it.
*************** DefineEnum(CreateEnumStmt *stmt)
*** 1191,1196 ****
--- 1192,1251 ----
pfree(enumArrayName);
}
+ /*
+ * AlterEnum
+ * Adds a new label to an existing enum.
+ */
+ void
+ AlterEnum(AlterEnumStmt *stmt)
+ {
+ Oid enum_type_oid;
+ TypeName *typename;
+ HeapTuple tup;
+
+ /* Make a TypeName so we can use standard type lookup machinery */
+ typename = makeTypeNameFromNameList(stmt->typeName);
+ enum_type_oid = typenameTypeId(NULL, typename, NULL);
+
+ tup = SearchSysCache1(TYPEOID, ObjectIdGetDatum(enum_type_oid));
+ if (!HeapTupleIsValid(tup))
+ elog(ERROR, "cache lookup failed for type %u", enum_type_oid);
+
+ /* Check it's an enum and check user has permission to ALTER the enum */
+ checkEnumOwner(tup);
+
+ /* Add the new label */
+ AddEnumLabel(enum_type_oid, stmt->newVal,
+ stmt->newValNeighbor, stmt->newValIsAfter);
+
+ ReleaseSysCache(tup);
+ }
+
+
+ /*
+ * checkEnumOwner
+ *
+ * Check that the type is actually an enum and that the current user
+ * has permission to do ALTER TYPE on it. Throw an error if not.
+ */
+ static void
+ checkEnumOwner(HeapTuple tup)
+ {
+ Form_pg_type typTup = (Form_pg_type) GETSTRUCT(tup);
+
+ /* Check that this is actually an enum */
+ if (typTup->typtype != TYPTYPE_ENUM)
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("%s is not an enum",
+ format_type_be(HeapTupleGetOid(tup)))));
+
+ /* Permission check: must own type */
+ if (!pg_type_ownercheck(HeapTupleGetOid(tup), GetUserId()))
+ aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_TYPE,
+ format_type_be(HeapTupleGetOid(tup)));
+ }
+
/*
* Find suitable I/O functions for a type.
*************** AlterDomainDefault(List *names, Node *de
*** 1576,1582 ****
typTup = (Form_pg_type) GETSTRUCT(tup);
/* Check it's a domain and check user has permission for ALTER DOMAIN */
! checkDomainOwner(tup, typename);
/* Setup new tuple */
MemSet(new_record, (Datum) 0, sizeof(new_record));
--- 1631,1637 ----
typTup = (Form_pg_type) GETSTRUCT(tup);
/* Check it's a domain and check user has permission for ALTER DOMAIN */
! checkDomainOwner(tup);
/* Setup new tuple */
MemSet(new_record, (Datum) 0, sizeof(new_record));
*************** AlterDomainNotNull(List *names, bool not
*** 1702,1708 ****
typTup = (Form_pg_type) GETSTRUCT(tup);
/* Check it's a domain and check user has permission for ALTER DOMAIN */
! checkDomainOwner(tup, typename);
/* Is the domain already set to the desired constraint? */
if (typTup->typnotnull == notNull)
--- 1757,1763 ----
typTup = (Form_pg_type) GETSTRUCT(tup);
/* Check it's a domain and check user has permission for ALTER DOMAIN */
! checkDomainOwner(tup);
/* Is the domain already set to the desired constraint? */
if (typTup->typnotnull == notNull)
*************** AlterDomainDropConstraint(List *names, c
*** 1801,1807 ****
elog(ERROR, "cache lookup failed for type %u", domainoid);
/* Check it's a domain and check user has permission for ALTER DOMAIN */
! checkDomainOwner(tup, typename);
/* Grab an appropriate lock on the pg_constraint relation */
conrel = heap_open(ConstraintRelationId, RowExclusiveLock);
--- 1856,1862 ----
elog(ERROR, "cache lookup failed for type %u", domainoid);
/* Check it's a domain and check user has permission for ALTER DOMAIN */
! checkDomainOwner(tup);
/* Grab an appropriate lock on the pg_constraint relation */
conrel = heap_open(ConstraintRelationId, RowExclusiveLock);
*************** AlterDomainAddConstraint(List *names, No
*** 1875,1881 ****
typTup = (Form_pg_type) GETSTRUCT(tup);
/* Check it's a domain and check user has permission for ALTER DOMAIN */
! checkDomainOwner(tup, typename);
if (!IsA(newConstraint, Constraint))
elog(ERROR, "unrecognized node type: %d",
--- 1930,1936 ----
typTup = (Form_pg_type) GETSTRUCT(tup);
/* Check it's a domain and check user has permission for ALTER DOMAIN */
! checkDomainOwner(tup);
if (!IsA(newConstraint, Constraint))
elog(ERROR, "unrecognized node type: %d",
*************** get_rels_with_domain(Oid domainOid, LOCK
*** 2187,2193 ****
* has permission to do ALTER DOMAIN on it. Throw an error if not.
*/
static void
! checkDomainOwner(HeapTuple tup, TypeName *typename)
{
Form_pg_type typTup = (Form_pg_type) GETSTRUCT(tup);
--- 2242,2248 ----
* has permission to do ALTER DOMAIN on it. Throw an error if not.
*/
static void
! checkDomainOwner(HeapTuple tup)
{
Form_pg_type typTup = (Form_pg_type) GETSTRUCT(tup);
*************** checkDomainOwner(HeapTuple tup, TypeName
*** 2195,2202 ****
if (typTup->typtype != TYPTYPE_DOMAIN)
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
! errmsg("\"%s\" is not a domain",
! TypeNameToString(typename))));
/* Permission check: must own type */
if (!pg_type_ownercheck(HeapTupleGetOid(tup), GetUserId()))
--- 2250,2257 ----
if (typTup->typtype != TYPTYPE_DOMAIN)
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
! errmsg("%s is not a domain",
! format_type_be(HeapTupleGetOid(tup)))));
/* Permission check: must own type */
if (!pg_type_ownercheck(HeapTupleGetOid(tup), GetUserId()))
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 508d7c70b137fa34f5dc31c00ec8fdf36de21a95..5346c72cd98f341e15b37b6d1ba75168ae5b0058 100644
*** a/src/backend/nodes/copyfuncs.c
--- b/src/backend/nodes/copyfuncs.c
*************** _copyCreateEnumStmt(CreateEnumStmt *from
*** 2901,2906 ****
--- 2901,2919 ----
return newnode;
}
+ static AlterEnumStmt *
+ _copyAlterEnumStmt(AlterEnumStmt *from)
+ {
+ AlterEnumStmt *newnode = makeNode(AlterEnumStmt);
+
+ COPY_NODE_FIELD(typeName);
+ COPY_STRING_FIELD(newVal);
+ COPY_STRING_FIELD(newValNeighbor);
+ COPY_SCALAR_FIELD(newValIsAfter);
+
+ return newnode;
+ }
+
static ViewStmt *
_copyViewStmt(ViewStmt *from)
{
*************** copyObject(void *from)
*** 4064,4069 ****
--- 4077,4085 ----
case T_CreateEnumStmt:
retval = _copyCreateEnumStmt(from);
break;
+ case T_AlterEnumStmt:
+ retval = _copyAlterEnumStmt(from);
+ break;
case T_ViewStmt:
retval = _copyViewStmt(from);
break;
diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index 19262aad6691ea8caca2c7a20fa07a2de0b4d59f..7cb2192d94725734405377864c36fc3c0271e2b7 100644
*** a/src/backend/nodes/equalfuncs.c
--- b/src/backend/nodes/equalfuncs.c
*************** _equalCreateEnumStmt(CreateEnumStmt *a,
*** 1393,1398 ****
--- 1393,1409 ----
}
static bool
+ _equalAlterEnumStmt(AlterEnumStmt *a, AlterEnumStmt *b)
+ {
+ COMPARE_NODE_FIELD(typeName);
+ COMPARE_STRING_FIELD(newVal);
+ COMPARE_STRING_FIELD(newValNeighbor);
+ COMPARE_SCALAR_FIELD(newValIsAfter);
+
+ return true;
+ }
+
+ static bool
_equalViewStmt(ViewStmt *a, ViewStmt *b)
{
COMPARE_NODE_FIELD(view);
*************** equal(void *a, void *b)
*** 2700,2705 ****
--- 2711,2719 ----
case T_CreateEnumStmt:
retval = _equalCreateEnumStmt(a, b);
break;
+ case T_AlterEnumStmt:
+ retval = _equalAlterEnumStmt(a, b);
+ break;
case T_ViewStmt:
retval = _equalViewStmt(a, b);
break;
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index c4165f0bf074f1be23e4bbdb4608fec2cbd76563..1394b21dec4de7400a466269ba6b36bb6390ce4e 100644
*** a/src/backend/parser/gram.y
--- b/src/backend/parser/gram.y
*************** static RangeVar *makeRangeVarFromAnyName
*** 182,189 ****
}
%type <node> stmt schema_stmt
! AlterDatabaseStmt AlterDatabaseSetStmt AlterDomainStmt AlterFdwStmt
! AlterForeignServerStmt AlterGroupStmt
AlterObjectSchemaStmt AlterOwnerStmt AlterSeqStmt AlterTableStmt
AlterCompositeTypeStmt AlterUserStmt AlterUserMappingStmt AlterUserSetStmt
AlterRoleStmt AlterRoleSetStmt
--- 182,189 ----
}
%type <node> stmt schema_stmt
! AlterDatabaseStmt AlterDatabaseSetStmt AlterDomainStmt AlterEnumStmt
! AlterFdwStmt AlterForeignServerStmt AlterGroupStmt
AlterObjectSchemaStmt AlterOwnerStmt AlterSeqStmt AlterTableStmt
AlterCompositeTypeStmt AlterUserStmt AlterUserMappingStmt AlterUserSetStmt
AlterRoleStmt AlterRoleSetStmt
*************** stmt :
*** 652,657 ****
--- 652,658 ----
| AlterDatabaseSetStmt
| AlterDefaultPrivilegesStmt
| AlterDomainStmt
+ | AlterEnumStmt
| AlterFdwStmt
| AlterForeignServerStmt
| AlterFunctionStmt
*************** enum_val_list: Sconst
*** 3863,3868 ****
--- 3864,3905 ----
{ $$ = lappend($1, makeString($3)); }
;
+ /*****************************************************************************
+ *
+ * ALTER TYPE enumtype ADD ...
+ *
+ *****************************************************************************/
+
+ AlterEnumStmt:
+ ALTER TYPE_P any_name ADD_P Sconst
+ {
+ AlterEnumStmt *n = makeNode(AlterEnumStmt);
+ n->typeName = $3;
+ n->newVal = $5;
+ n->newValNeighbor = NULL;
+ n->newValIsAfter = true;
+ $$ = (Node *) n;
+ }
+ | ALTER TYPE_P any_name ADD_P Sconst BEFORE Sconst
+ {
+ AlterEnumStmt *n = makeNode(AlterEnumStmt);
+ n->typeName = $3;
+ n->newVal = $5;
+ n->newValNeighbor = $7;
+ n->newValIsAfter = false;
+ $$ = (Node *) n;
+ }
+ | ALTER TYPE_P any_name ADD_P Sconst AFTER Sconst
+ {
+ AlterEnumStmt *n = makeNode(AlterEnumStmt);
+ n->typeName = $3;
+ n->newVal = $5;
+ n->newValNeighbor = $7;
+ n->newValIsAfter = true;
+ $$ = (Node *) n;
+ }
+ ;
+
/*****************************************************************************
*
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index 75cb354ea89db9ce071ab3d781c8daf3280cd5e4..2c8b7aa810ef12101ee0755b28f391f0162ab6e2 100644
*** a/src/backend/tcop/utility.c
--- b/src/backend/tcop/utility.c
*************** check_xact_readonly(Node *parsetree)
*** 190,195 ****
--- 190,196 ----
case T_CreateTrigStmt:
case T_CompositeTypeStmt:
case T_CreateEnumStmt:
+ case T_AlterEnumStmt:
case T_ViewStmt:
case T_DropCastStmt:
case T_DropStmt:
*************** standard_ProcessUtility(Node *parsetree,
*** 860,865 ****
--- 861,870 ----
DefineEnum((CreateEnumStmt *) parsetree);
break;
+ case T_AlterEnumStmt: /* ALTER TYPE (enum) */
+ AlterEnum((AlterEnumStmt *) parsetree);
+ break;
+
case T_ViewStmt: /* CREATE VIEW */
DefineView((ViewStmt *) parsetree, queryString);
break;
*************** CreateCommandTag(Node *parsetree)
*** 1868,1873 ****
--- 1873,1882 ----
tag = "CREATE TYPE";
break;
+ case T_AlterEnumStmt:
+ tag = "ALTER TYPE";
+ break;
+
case T_ViewStmt:
tag = "CREATE VIEW";
break;
*************** GetCommandLogLevel(Node *parsetree)
*** 2410,2415 ****
--- 2419,2428 ----
lev = LOGSTMT_DDL;
break;
+ case T_AlterEnumStmt:
+ lev = LOGSTMT_DDL;
+ break;
+
case T_ViewStmt:
lev = LOGSTMT_DDL;
break;
diff --git a/src/backend/utils/adt/enum.c b/src/backend/utils/adt/enum.c
index e5747a46bcd341488b26cc739f50dc0f8d14f622..ff9fa498f296ffb9f626a55d58d641931f2c57d2 100644
*** a/src/backend/utils/adt/enum.c
--- b/src/backend/utils/adt/enum.c
***************
*** 15,30 ****
#include "catalog/pg_enum.h"
#include "fmgr.h"
#include "utils/array.h"
#include "utils/builtins.h"
#include "utils/lsyscache.h"
#include "utils/syscache.h"
! #include "libpq/pqformat.h"
! #include "miscadmin.h"
static ArrayType *enum_range_internal(Oid enumtypoid, Oid lower, Oid upper);
! static int enum_elem_cmp(const void *left, const void *right);
/* Basic I/O support */
--- 15,37 ----
#include "catalog/pg_enum.h"
#include "fmgr.h"
+ #include "libpq/pqformat.h"
+ #include "miscadmin.h"
#include "utils/array.h"
#include "utils/builtins.h"
#include "utils/lsyscache.h"
#include "utils/syscache.h"
! #include "utils/typcache.h"
+ typedef struct
+ {
+ Oid enum_oid; /* OID of one enum value */
+ float4 sort_order; /* its sort position */
+ } enum_sort;
+
static ArrayType *enum_range_internal(Oid enumtypoid, Oid lower, Oid upper);
! static int enum_sort_cmp(const void *left, const void *right);
/* Basic I/O support */
*************** enum_send(PG_FUNCTION_ARGS)
*** 155,167 ****
/* Comparison functions and related */
Datum
enum_lt(PG_FUNCTION_ARGS)
{
Oid a = PG_GETARG_OID(0);
Oid b = PG_GETARG_OID(1);
! PG_RETURN_BOOL(a < b);
}
Datum
--- 162,224 ----
/* Comparison functions and related */
+ /*
+ * enum_cmp_internal is the common engine for all the visible comparison
+ * functions, except for enum_eq and enum_ne which can just check for OID
+ * equality directly.
+ */
+ static int
+ enum_cmp_internal(Oid arg1, Oid arg2, FunctionCallInfo fcinfo)
+ {
+ TypeCacheEntry *tcache;
+
+ /* Equal OIDs are equal no matter what */
+ if (arg1 == arg2)
+ return 0;
+
+ /* Fast path: even-numbered Oids are known to compare correctly */
+ if ((arg1 & 1) == 0 && (arg2 & 1) == 0)
+ {
+ if (arg1 < arg2)
+ return -1;
+ else
+ return 1;
+ }
+
+ /* Locate the typcache entry for the enum type */
+ tcache = (TypeCacheEntry *) fcinfo->flinfo->fn_extra;
+ if (tcache == NULL)
+ {
+ HeapTuple enum_tup;
+ Form_pg_enum en;
+ Oid typeoid;
+
+ /* Get the OID of the enum type containing arg1 */
+ enum_tup = SearchSysCache1(ENUMOID, ObjectIdGetDatum(arg1));
+ if (!HeapTupleIsValid(enum_tup))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_BINARY_REPRESENTATION),
+ errmsg("invalid internal value for enum: %u",
+ arg1)));
+ en = (Form_pg_enum) GETSTRUCT(enum_tup);
+ typeoid = en->enumtypid;
+ ReleaseSysCache(enum_tup);
+ /* Now locate and remember the typcache entry */
+ tcache = lookup_type_cache(typeoid, 0);
+ fcinfo->flinfo->fn_extra = (void *) tcache;
+ }
+
+ /* The remaining comparison logic is in typcache.c */
+ return compare_values_of_enum(tcache, arg1, arg2);
+ }
+
Datum
enum_lt(PG_FUNCTION_ARGS)
{
Oid a = PG_GETARG_OID(0);
Oid b = PG_GETARG_OID(1);
! PG_RETURN_BOOL(enum_cmp_internal(a, b, fcinfo) < 0);
}
Datum
*************** enum_le(PG_FUNCTION_ARGS)
*** 170,176 ****
Oid a = PG_GETARG_OID(0);
Oid b = PG_GETARG_OID(1);
! PG_RETURN_BOOL(a <= b);
}
Datum
--- 227,233 ----
Oid a = PG_GETARG_OID(0);
Oid b = PG_GETARG_OID(1);
! PG_RETURN_BOOL(enum_cmp_internal(a, b, fcinfo) <= 0);
}
Datum
*************** enum_ge(PG_FUNCTION_ARGS)
*** 197,203 ****
Oid a = PG_GETARG_OID(0);
Oid b = PG_GETARG_OID(1);
! PG_RETURN_BOOL(a >= b);
}
Datum
--- 254,260 ----
Oid a = PG_GETARG_OID(0);
Oid b = PG_GETARG_OID(1);
! PG_RETURN_BOOL(enum_cmp_internal(a, b, fcinfo) >= 0);
}
Datum
*************** enum_gt(PG_FUNCTION_ARGS)
*** 206,212 ****
Oid a = PG_GETARG_OID(0);
Oid b = PG_GETARG_OID(1);
! PG_RETURN_BOOL(a > b);
}
Datum
--- 263,269 ----
Oid a = PG_GETARG_OID(0);
Oid b = PG_GETARG_OID(1);
! PG_RETURN_BOOL(enum_cmp_internal(a, b, fcinfo) > 0);
}
Datum
*************** enum_smaller(PG_FUNCTION_ARGS)
*** 215,221 ****
Oid a = PG_GETARG_OID(0);
Oid b = PG_GETARG_OID(1);
! PG_RETURN_OID(a <= b ? a : b);
}
Datum
--- 272,278 ----
Oid a = PG_GETARG_OID(0);
Oid b = PG_GETARG_OID(1);
! PG_RETURN_OID(enum_cmp_internal(a, b, fcinfo) < 0 ? a : b);
}
Datum
*************** enum_larger(PG_FUNCTION_ARGS)
*** 224,230 ****
Oid a = PG_GETARG_OID(0);
Oid b = PG_GETARG_OID(1);
! PG_RETURN_OID(a >= b ? a : b);
}
Datum
--- 281,287 ----
Oid a = PG_GETARG_OID(0);
Oid b = PG_GETARG_OID(1);
! PG_RETURN_OID(enum_cmp_internal(a, b, fcinfo) > 0 ? a : b);
}
Datum
*************** enum_cmp(PG_FUNCTION_ARGS)
*** 233,242 ****
Oid a = PG_GETARG_OID(0);
Oid b = PG_GETARG_OID(1);
! if (a > b)
! PG_RETURN_INT32(1);
! else if (a == b)
PG_RETURN_INT32(0);
else
PG_RETURN_INT32(-1);
}
--- 290,299 ----
Oid a = PG_GETARG_OID(0);
Oid b = PG_GETARG_OID(1);
! if (a == b)
PG_RETURN_INT32(0);
+ else if (enum_cmp_internal(a, b, fcinfo) > 0)
+ PG_RETURN_INT32(1);
else
PG_RETURN_INT32(-1);
}
*************** enum_first(PG_FUNCTION_ARGS)
*** 248,253 ****
--- 305,311 ----
{
Oid enumtypoid;
Oid min = InvalidOid;
+ float4 min_sort = 0; /* arbitrary, this value won't be used */
CatCList *list;
int num,
i;
*************** enum_first(PG_FUNCTION_ARGS)
*** 267,276 ****
num = list->n_members;
for (i = 0; i < num; i++)
{
! Oid valoid = HeapTupleHeaderGetOid(list->members[i]->tuple.t_data);
! if (!OidIsValid(min) || valoid < min)
! min = valoid;
}
ReleaseCatCacheList(list);
--- 325,338 ----
num = list->n_members;
for (i = 0; i < num; i++)
{
! HeapTuple tup = &(list->members[i]->tuple);
! Form_pg_enum en = (Form_pg_enum) GETSTRUCT(tup);
! if (!OidIsValid(min) || en->enumsortorder < min_sort)
! {
! min = HeapTupleGetOid(tup);
! min_sort = en->enumsortorder;
! }
}
ReleaseCatCacheList(list);
*************** enum_last(PG_FUNCTION_ARGS)
*** 287,292 ****
--- 349,355 ----
{
Oid enumtypoid;
Oid max = InvalidOid;
+ float4 max_sort = 0; /* arbitrary, this value won't be used */
CatCList *list;
int num,
i;
*************** enum_last(PG_FUNCTION_ARGS)
*** 306,315 ****
num = list->n_members;
for (i = 0; i < num; i++)
{
! Oid valoid = HeapTupleHeaderGetOid(list->members[i]->tuple.t_data);
! if (!OidIsValid(max) || valoid > max)
! max = valoid;
}
ReleaseCatCacheList(list);
--- 369,382 ----
num = list->n_members;
for (i = 0; i < num; i++)
{
! HeapTuple tup = &(list->members[i]->tuple);
! Form_pg_enum en = (Form_pg_enum) GETSTRUCT(tup);
! if (!OidIsValid(max) || en->enumsortorder > max_sort)
! {
! max = HeapTupleGetOid(tup);
! max_sort = en->enumsortorder;
! }
}
ReleaseCatCacheList(list);
*************** enum_range_internal(Oid enumtypoid, Oid
*** 382,427 ****
i,
j;
Datum *elems;
list = SearchSysCacheList1(ENUMTYPOIDNAME, ObjectIdGetDatum(enumtypoid));
total = list->n_members;
elems = (Datum *) palloc(total * sizeof(Datum));
- j = 0;
for (i = 0; i < total; i++)
{
! Oid val = HeapTupleGetOid(&(list->members[i]->tuple));
! if ((!OidIsValid(lower) || lower <= val) &&
! (!OidIsValid(upper) || val <= upper))
! elems[j++] = ObjectIdGetDatum(val);
}
! /* shouldn't need the cache anymore */
ReleaseCatCacheList(list);
! /* sort results into OID order */
! qsort(elems, j, sizeof(Datum), enum_elem_cmp);
/* note this hardwires some details about the representation of Oid */
result = construct_array(elems, j, enumtypoid, sizeof(Oid), true, 'i');
pfree(elems);
return result;
}
! /* qsort comparison function for Datums that are OIDs */
static int
! enum_elem_cmp(const void *left, const void *right)
{
! Oid l = DatumGetObjectId(*((const Datum *) left));
! Oid r = DatumGetObjectId(*((const Datum *) right));
! if (l < r)
return -1;
! if (l > r)
return 1;
! return 0;
}
--- 449,517 ----
i,
j;
Datum *elems;
+ enum_sort *sort_items;
+ bool left_found;
+ /* collect information about the enum's members using the syscache */
list = SearchSysCacheList1(ENUMTYPOIDNAME, ObjectIdGetDatum(enumtypoid));
total = list->n_members;
elems = (Datum *) palloc(total * sizeof(Datum));
+ sort_items = (enum_sort *) palloc(total * sizeof(enum_sort));
for (i = 0; i < total; i++)
{
! HeapTuple tup = &(list->members[i]->tuple);
! Form_pg_enum en = (Form_pg_enum) GETSTRUCT(tup);
! sort_items[i].enum_oid = HeapTupleGetOid(tup);
! sort_items[i].sort_order = en->enumsortorder;
}
! /* shouldn't need the syscache items anymore */
ReleaseCatCacheList(list);
! /* sort results into sort_order sequence */
! qsort(sort_items, total, sizeof(enum_sort), enum_sort_cmp);
!
! /* collect the desired OIDs in elems[] */
! j = 0;
! left_found = !OidIsValid(lower);
! for (i = 0; i < total; i++)
! {
! if (!left_found && lower == sort_items[i].enum_oid)
! left_found = true;
!
! if (left_found)
! elems[j++] = ObjectIdGetDatum(sort_items[i].enum_oid);
!
! if (OidIsValid(upper) && upper == sort_items[i].enum_oid)
! break;
! }
+ /* and build the result array */
/* note this hardwires some details about the representation of Oid */
result = construct_array(elems, j, enumtypoid, sizeof(Oid), true, 'i');
pfree(elems);
+ pfree(sort_items);
return result;
}
! /*
! * qsort comparison using sort order, for range routines
! */
static int
! enum_sort_cmp(const void *left, const void *right)
{
! const enum_sort *l = (const enum_sort *) left;
! const enum_sort *r = (const enum_sort *) right;
! if (l->sort_order < r->sort_order)
return -1;
! else if (l->sort_order > r->sort_order)
return 1;
! else
! return 0;
}
diff --git a/src/backend/utils/cache/typcache.c b/src/backend/utils/cache/typcache.c
index 200961448903b12cabb11b895629404ca0b3d90a..e683203fd4e24d18a043849f9cd96f757a19faab 100644
*** a/src/backend/utils/cache/typcache.c
--- b/src/backend/utils/cache/typcache.c
***************
*** 45,63 ****
--- 45,83 ----
#include "access/hash.h"
#include "access/heapam.h"
#include "access/nbtree.h"
+ #include "catalog/indexing.h"
+ #include "catalog/pg_enum.h"
#include "catalog/pg_type.h"
#include "commands/defrem.h"
#include "utils/builtins.h"
+ #include "utils/fmgroids.h"
#include "utils/inval.h"
#include "utils/lsyscache.h"
#include "utils/rel.h"
+ #include "utils/snapmgr.h"
#include "utils/syscache.h"
+ #include "utils/tqual.h"
#include "utils/typcache.h"
/* The main type cache hashtable searched by lookup_type_cache */
static HTAB *TypeCacheHash = NULL;
+ /* Private information to support comparisons of enum values */
+ typedef struct
+ {
+ Oid enum_oid; /* OID of one enum value */
+ float4 sort_order; /* its sort position */
+ } EnumItem;
+
+ typedef struct TypeCacheEnumData
+ {
+ Oid bitmap_base; /* OID corresponding to bit 0 of bitmapset */
+ Bitmapset *sorted_values; /* Set of OIDs known to be in order */
+ int num_values; /* total number of values in enum */
+ EnumItem enum_values[1]; /* VARIABLE LENGTH ARRAY */
+ } TypeCacheEnumData;
+
/*
* We use a separate table for storing the definitions of non-anonymous
* record types. Once defined, a record type will be remembered for the
*************** static int32 RecordCacheArrayLen = 0; /*
*** 88,93 ****
--- 108,116 ----
static int32 NextRecordTypmod = 0; /* number of entries used */
static void TypeCacheRelCallback(Datum arg, Oid relid);
+ static void load_enum_cache_data(TypeCacheEntry *tcache);
+ static EnumItem *find_enumitem(TypeCacheEnumData *enumdata, Oid arg);
+ static int enum_oid_cmp(const void *left, const void *right);
/*
*************** TypeCacheRelCallback(Datum arg, Oid reli
*** 551,553 ****
--- 574,868 ----
}
}
}
+
+
+ /*
+ * compare_values_of_enum
+ * Compare two members of an enum type.
+ * Return <0, 0, or >0 according as arg1 <, =, or > arg2.
+ *
+ * Note: currently, the enumData cache is refreshed only if we are asked
+ * to compare an enum value that is not already in the cache. This is okay
+ * because there is no support for re-ordering existing values, so comparisons
+ * of previously cached values will return the right answer even if other
+ * values have been added since we last loaded the cache.
+ *
+ * Note: the enum logic has a special-case rule about even-numbered versus
+ * odd-numbered OIDs, but we take no account of that rule here; this
+ * routine shouldn't even get called when that rule applies.
+ */
+ int
+ compare_values_of_enum(TypeCacheEntry *tcache, Oid arg1, Oid arg2)
+ {
+ TypeCacheEnumData *enumdata;
+ EnumItem *item1;
+ EnumItem *item2;
+
+ /*
+ * Equal OIDs are certainly equal --- this case was probably handled
+ * by our caller, but we may as well check.
+ */
+ if (arg1 == arg2)
+ return 0;
+
+ /* Load up the cache if first time through */
+ if (tcache->enumData == NULL)
+ load_enum_cache_data(tcache);
+ enumdata = tcache->enumData;
+
+ /*
+ * If both OIDs are known-sorted, we can just compare them directly.
+ * (Note: this coding assumes that OID is no wider than int, else we
+ * would have to do an explicit range-check.)
+ */
+ if (bms_is_member(arg1 - enumdata->bitmap_base, enumdata->sorted_values) &&
+ bms_is_member(arg2 - enumdata->bitmap_base, enumdata->sorted_values))
+ {
+ if (arg1 < arg2)
+ return -1;
+ else
+ return 1;
+ }
+
+ /*
+ * Slow path: we have to identify their actual sort-order positions.
+ */
+ item1 = find_enumitem(enumdata, arg1);
+ item2 = find_enumitem(enumdata, arg2);
+
+ if (item1 == NULL || item2 == NULL)
+ {
+ /*
+ * We couldn't find one or both values. That means the enum has
+ * changed under us, so re-initialize the cache and try again.
+ * We don't bother retrying the known-sorted case in this path.
+ */
+ load_enum_cache_data(tcache);
+ enumdata = tcache->enumData;
+
+ item1 = find_enumitem(enumdata, arg1);
+ item2 = find_enumitem(enumdata, arg2);
+
+ /*
+ * If we still can't find the values, complain: we must have
+ * corrupt data.
+ */
+ if (item1 == NULL)
+ elog(ERROR, "enum value %u not found in cache for enum %s",
+ arg1, format_type_be(tcache->type_id));
+ if (item2 == NULL)
+ elog(ERROR, "enum value %u not found in cache for enum %s",
+ arg2, format_type_be(tcache->type_id));
+ }
+
+ if (item1->sort_order < item2->sort_order)
+ return -1;
+ else if (item1->sort_order > item2->sort_order)
+ return 1;
+ else
+ return 0;
+ }
+
+ /*
+ * Load (or re-load) the enumData member of the typcache entry.
+ */
+ static void
+ load_enum_cache_data(TypeCacheEntry *tcache)
+ {
+ TypeCacheEnumData *enumdata;
+ Relation enum_rel;
+ SysScanDesc enum_scan;
+ HeapTuple enum_tuple;
+ ScanKeyData skey;
+ EnumItem *items;
+ int numitems;
+ int maxitems;
+ Oid bitmap_base;
+ Bitmapset *bms;
+ MemoryContext oldcxt;
+ int i,
+ bm_start,
+ bm_len,
+ start_pos,
+ list_end;
+
+ /* Check that this is actually an enum */
+ if (tcache->typtype != TYPTYPE_ENUM)
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("%s is not an enum",
+ format_type_be(tcache->type_id))));
+
+ /*
+ * Read all the information for members of the enum type. We collect
+ * the info in working memory in the caller's context, and then transfer
+ * it to permanent memory in CacheMemoryContext. This minimizes the risk
+ * of leaking memory from CacheMemoryContext in the event of an error
+ * partway through.
+ */
+ maxitems = 64;
+ items = (EnumItem *) palloc(sizeof(EnumItem) * maxitems);
+ numitems = 0;
+
+ /*
+ * Scan pg_enum for the members of the target enum type. We use a
+ * current MVCC snapshot, *not* SnapshotNow, so that we see a consistent
+ * set of rows even if someone commits a renumbering of the enum meanwhile.
+ * See comments in catalog/pg_enum.c for more info.
+ */
+ ScanKeyInit(&skey,
+ Anum_pg_enum_enumtypid,
+ BTEqualStrategyNumber, F_OIDEQ,
+ ObjectIdGetDatum(tcache->type_id));
+
+ enum_rel = heap_open(EnumRelationId, AccessShareLock);
+ enum_scan = systable_beginscan(enum_rel,
+ EnumTypIdLabelIndexId,
+ true, GetTransactionSnapshot(),
+ 1, &skey);
+
+ while (HeapTupleIsValid(enum_tuple = systable_getnext(enum_scan)))
+ {
+ Form_pg_enum en = (Form_pg_enum) GETSTRUCT(enum_tuple);
+
+ if (numitems >= maxitems)
+ {
+ maxitems *= 2;
+ items = (EnumItem *) repalloc(items, sizeof(EnumItem) * maxitems);
+ }
+ items[numitems].enum_oid = HeapTupleGetOid(enum_tuple);
+ items[numitems].sort_order = en->enumsortorder;
+ numitems++;
+ }
+
+ systable_endscan(enum_scan);
+ heap_close(enum_rel, AccessShareLock);
+
+ /* Sort the items into OID order */
+ qsort(items, numitems, sizeof(EnumItem), enum_oid_cmp);
+
+ /*
+ * Here, we create a bitmap listing a subset of the enum's OIDs that are
+ * known to be in order and can thus be compared with just OID comparison.
+ *
+ * The point of this is that the enum's initial OIDs were certainly in
+ * order, so there is some subset that can be compared via OID comparison;
+ * and we'd rather not do binary searches unnecessarily.
+ */
+
+ /* Look for the longest suitable range for the bitmap */
+ bm_len = 0;
+ bm_start = 0;
+
+ for (start_pos = 0; start_pos < numitems - 1; start_pos++)
+ {
+ /*
+ * Identify longest sorted subsequence starting at start_pos
+ */
+ Oid start_oid = items[start_pos].enum_oid;
+
+ for (list_end = start_pos + 1; list_end < numitems; list_end++)
+ {
+ /* quit if list_end item is out-of-order */
+ if (items[list_end].sort_order <= items[list_end - 1].sort_order)
+ break;
+ /* quit if bitmap would be too large; cutoff is arbitrary */
+ if ((items[list_end].enum_oid - start_oid) >= 1024)
+ break;
+ }
+
+ /* Remember it if larger than previous best */
+ if ((list_end - start_pos) > bm_len)
+ {
+ bm_len = list_end - start_pos;
+ bm_start = start_pos;
+
+ /*
+ * If the sequence we've just found is bigger than half the list,
+ * we won't find one bigger
+ */
+ if (bm_len > numitems / 2)
+ break;
+ }
+
+ /*
+ * If we stopped at an out-of-order item, there's no point in starting
+ * another scan before that item.
+ */
+ if (list_end < numitems &&
+ items[list_end].sort_order <= items[list_end - 1].sort_order)
+ start_pos = list_end - 1;
+ }
+
+ /* If we found a suitable range for the bitmap, set it up */
+ if (bm_len > 1)
+ {
+ bitmap_base = items[bm_start].enum_oid;
+ bms = NULL;
+ for (i = 0; i < bm_len; i++)
+ {
+ Oid en_oid = items[bm_start + i].enum_oid;
+
+ bms = bms_add_member(bms, (int) (en_oid - bitmap_base));
+ }
+ }
+ else
+ {
+ /* This should be an unusual corner case */
+ bitmap_base = InvalidOid;
+ bms = NULL;
+ }
+
+ /* OK, copy the data into CacheMemoryContext */
+ oldcxt = MemoryContextSwitchTo(CacheMemoryContext);
+ enumdata = (TypeCacheEnumData *)
+ palloc(offsetof(TypeCacheEnumData, enum_values) +
+ numitems * sizeof(EnumItem));
+ enumdata->bitmap_base = bitmap_base;
+ enumdata->sorted_values = bms_copy(bms);
+ enumdata->num_values = numitems;
+ memcpy(enumdata->enum_values, items, numitems * sizeof(EnumItem));
+ MemoryContextSwitchTo(oldcxt);
+
+ pfree(items);
+ bms_free(bms);
+
+ /* And link the finished cache struct into the typcache */
+ if (tcache->enumData != NULL)
+ pfree(tcache->enumData);
+ tcache->enumData = enumdata;
+ }
+
+ /*
+ * Locate the EnumItem with the given OID, if present
+ */
+ static EnumItem *
+ find_enumitem(TypeCacheEnumData *enumdata, Oid arg)
+ {
+ EnumItem srch;
+
+ /* On some versions of Solaris, bsearch of zero items dumps core */
+ if (enumdata->num_values <= 0)
+ return NULL;
+
+ srch.enum_oid = arg;
+ return bsearch(&srch, enumdata->enum_values, enumdata->num_values,
+ sizeof(EnumItem), enum_oid_cmp);
+ }
+
+ /*
+ * qsort comparison function for OID-ordered EnumItems
+ */
+ static int
+ enum_oid_cmp(const void *left, const void *right)
+ {
+ const EnumItem *l = (const EnumItem *) left;
+ const EnumItem *r = (const EnumItem *) right;
+
+ if (l->enum_oid < r->enum_oid)
+ return -1;
+ else if (l->enum_oid > r->enum_oid)
+ return 1;
+ else
+ return 0;
+ }
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 6a4557b48610f51247209b80ea5b8962f1fea048..55ea6841a443ca0558e526a895cfbf2f484d76ba 100644
*** a/src/bin/pg_dump/pg_dump.c
--- b/src/bin/pg_dump/pg_dump.c
*************** dumpEnumType(Archive *fout, TypeInfo *ty
*** 6657,6670 ****
Oid enum_oid;
char *label;
! /* Set proper schema search path so regproc references list correctly */
! selectSourceSchema(tyinfo->dobj.namespace->dobj.name);
! appendPQExpBuffer(query, "SELECT oid, enumlabel "
! "FROM pg_catalog.pg_enum "
! "WHERE enumtypid = '%u'"
! "ORDER BY oid",
! tyinfo->dobj.catId.oid);
res = PQexec(g_conn, query->data);
check_sql_result(res, g_conn, query->data, PGRES_TUPLES_OK);
--- 6657,6677 ----
Oid enum_oid;
char *label;
! /* Set proper schema search path */
! selectSourceSchema("pg_catalog");
! if (fout->remoteVersion >= 90100)
! appendPQExpBuffer(query, "SELECT oid, enumlabel "
! "FROM pg_catalog.pg_enum "
! "WHERE enumtypid = '%u'"
! "ORDER BY enumsortorder",
! tyinfo->dobj.catId.oid);
! else
! appendPQExpBuffer(query, "SELECT oid, enumlabel "
! "FROM pg_catalog.pg_enum "
! "WHERE enumtypid = '%u'"
! "ORDER BY oid",
! tyinfo->dobj.catId.oid);
res = PQexec(g_conn, query->data);
check_sql_result(res, g_conn, query->data, PGRES_TUPLES_OK);
*************** dumpEnumType(Archive *fout, TypeInfo *ty
*** 6713,6725 ****
if (i == 0)
appendPQExpBuffer(q, "\n-- For binary upgrade, must preserve pg_enum oids\n");
appendPQExpBuffer(q,
! "SELECT binary_upgrade.add_pg_enum_label('%u'::pg_catalog.oid, "
! "'%u'::pg_catalog.oid, ",
! enum_oid, tyinfo->dobj.catId.oid);
appendStringLiteralAH(q, label, fout);
! appendPQExpBuffer(q, ");\n");
}
- appendPQExpBuffer(q, "\n");
}
ArchiveEntry(fout, tyinfo->dobj.catId, tyinfo->dobj.dumpId,
--- 6720,6734 ----
if (i == 0)
appendPQExpBuffer(q, "\n-- For binary upgrade, must preserve pg_enum oids\n");
appendPQExpBuffer(q,
! "SELECT binary_upgrade.set_next_pg_enum_oid('%u'::pg_catalog.oid);\n",
! enum_oid);
! appendPQExpBuffer(q, "ALTER TYPE %s.",
! fmtId(tyinfo->dobj.namespace->dobj.name));
! appendPQExpBuffer(q, "%s ADD ",
! fmtId(tyinfo->dobj.name));
appendStringLiteralAH(q, label, fout);
! appendPQExpBuffer(q, ";\n\n");
}
}
ArchiveEntry(fout, tyinfo->dobj.catId, tyinfo->dobj.dumpId,
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 57d74e14d75e3556389cff966ab7fc360b97fdbd..b705cb29dd419b207a365ae61801f8a6512da253 100644
*** a/src/bin/psql/describe.c
--- b/src/bin/psql/describe.c
*************** describeTypes(const char *pattern, bool
*** 473,489 ****
gettext_noop("Internal name"),
gettext_noop("Size"));
if (verbose && pset.sversion >= 80300)
appendPQExpBuffer(&buf,
" pg_catalog.array_to_string(\n"
" ARRAY(\n"
" SELECT e.enumlabel\n"
" FROM pg_catalog.pg_enum e\n"
! " WHERE e.enumtypid = t.oid\n"
! " ORDER BY e.oid\n"
" ),\n"
" E'\\n'\n"
" ) AS \"%s\",\n",
gettext_noop("Elements"));
appendPQExpBuffer(&buf,
" pg_catalog.obj_description(t.oid, 'pg_type') as \"%s\"\n",
--- 473,499 ----
gettext_noop("Internal name"),
gettext_noop("Size"));
if (verbose && pset.sversion >= 80300)
+ {
appendPQExpBuffer(&buf,
" pg_catalog.array_to_string(\n"
" ARRAY(\n"
" SELECT e.enumlabel\n"
" FROM pg_catalog.pg_enum e\n"
! " WHERE e.enumtypid = t.oid\n");
!
! if (pset.sversion >= 90100)
! appendPQExpBuffer(&buf,
! " ORDER BY e.enumsortorder\n");
! else
! appendPQExpBuffer(&buf,
! " ORDER BY e.oid\n");
!
! appendPQExpBuffer(&buf,
" ),\n"
" E'\\n'\n"
" ) AS \"%s\",\n",
gettext_noop("Elements"));
+ }
appendPQExpBuffer(&buf,
" pg_catalog.obj_description(t.oid, 'pg_type') as \"%s\"\n",
diff --git a/src/include/catalog/catversion.h b/src/include/catalog/catversion.h
index e30a7d7298b489286c2e8cccea45a32de8ceb556..b858d3ee009626cea6258d5da775b307e0e0b995 100644
*** a/src/include/catalog/catversion.h
--- b/src/include/catalog/catversion.h
***************
*** 53,58 ****
*/
/* yyyymmddN */
! #define CATALOG_VERSION_NO 201010201
#endif
--- 53,58 ----
*/
/* yyyymmddN */
! #define CATALOG_VERSION_NO 201010241
#endif
diff --git a/src/include/catalog/indexing.h b/src/include/catalog/indexing.h
index b7c9849314eb1dc2f468ea0a3c22fe2e63de2b67..a3839e1e2597e3d5ccfd848c69265eb9057080e7 100644
*** a/src/include/catalog/indexing.h
--- b/src/include/catalog/indexing.h
*************** DECLARE_UNIQUE_INDEX(pg_enum_oid_index,
*** 147,152 ****
--- 147,154 ----
#define EnumOidIndexId 3502
DECLARE_UNIQUE_INDEX(pg_enum_typid_label_index, 3503, on pg_enum using btree(enumtypid oid_ops, enumlabel name_ops));
#define EnumTypIdLabelIndexId 3503
+ DECLARE_UNIQUE_INDEX(pg_enum_typid_sortorder_index, 3534, on pg_enum using btree(enumtypid oid_ops, enumsortorder
float4_ops));
+ #define EnumTypIdSortOrderIndexId 3534
/* This following index is not used for a cache and is not unique */
DECLARE_INDEX(pg_index_indrelid_index, 2678, on pg_index using btree(indrelid oid_ops));
diff --git a/src/include/catalog/pg_enum.h b/src/include/catalog/pg_enum.h
index 28da42bf54e417047c08b0a94c82e1baad61e37e..cc69eb531c6e558c87b41794543598452b0ec390 100644
*** a/src/include/catalog/pg_enum.h
--- b/src/include/catalog/pg_enum.h
***************
*** 34,39 ****
--- 34,40 ----
CATALOG(pg_enum,3501)
{
Oid enumtypid; /* OID of owning enum type */
+ float4 enumsortorder; /* sort position of this enum value */
NameData enumlabel; /* text representation of enum value */
} FormData_pg_enum;
*************** typedef FormData_pg_enum *Form_pg_enum;
*** 48,56 ****
* compiler constants for pg_enum
* ----------------
*/
! #define Natts_pg_enum 2
#define Anum_pg_enum_enumtypid 1
! #define Anum_pg_enum_enumlabel 2
/* ----------------
* pg_enum has no initial contents
--- 49,58 ----
* compiler constants for pg_enum
* ----------------
*/
! #define Natts_pg_enum 3
#define Anum_pg_enum_enumtypid 1
! #define Anum_pg_enum_enumsortorder 2
! #define Anum_pg_enum_enumlabel 3
/* ----------------
* pg_enum has no initial contents
*************** typedef FormData_pg_enum *Form_pg_enum;
*** 60,67 ****
/*
* prototypes for functions in pg_enum.c
*/
! extern void EnumValuesCreate(Oid enumTypeOid, List *vals,
! Oid binary_upgrade_next_pg_enum_oid);
extern void EnumValuesDelete(Oid enumTypeOid);
#endif /* PG_ENUM_H */
--- 62,70 ----
/*
* prototypes for functions in pg_enum.c
*/
! extern void EnumValuesCreate(Oid enumTypeOid, List *vals);
extern void EnumValuesDelete(Oid enumTypeOid);
+ extern void AddEnumLabel(Oid enumTypeOid, const char *newVal,
+ const char *neighbor, bool newValIsAfter);
#endif /* PG_ENUM_H */
diff --git a/src/include/commands/typecmds.h b/src/include/commands/typecmds.h
index 2bff7e1c2b138481d33aebfd2f03504815f7d02a..4c6e7e164e0a3da23333014e0f9bac9ca5c170df 100644
*** a/src/include/commands/typecmds.h
--- b/src/include/commands/typecmds.h
*************** extern void RemoveTypes(DropStmt *drop);
*** 24,29 ****
--- 24,30 ----
extern void RemoveTypeById(Oid typeOid);
extern void DefineDomain(CreateDomainStmt *stmt);
extern void DefineEnum(CreateEnumStmt *stmt);
+ extern void AlterEnum(AlterEnumStmt *stmt);
extern Oid DefineCompositeType(const RangeVar *typevar, List *coldeflist);
extern Oid AssignTypeArrayOid(void);
diff --git a/src/include/nodes/nodes.h b/src/include/nodes/nodes.h
index 15dabe31ce39750f35e77ebcaf274ff40df313a0..8e94d9803f702875cd47b85b48b1ebe29ac257b9 100644
*** a/src/include/nodes/nodes.h
--- b/src/include/nodes/nodes.h
*************** typedef enum NodeTag
*** 338,343 ****
--- 338,344 ----
T_ReassignOwnedStmt,
T_CompositeTypeStmt,
T_CreateEnumStmt,
+ T_AlterEnumStmt,
T_AlterTSDictionaryStmt,
T_AlterTSConfigurationStmt,
T_CreateFdwStmt,
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index e0bdebd0abe378727fcfb858bae31c503a8df6d0..3cac54b94708e574fc7f53bba4317be8d00b4855 100644
*** a/src/include/nodes/parsenodes.h
--- b/src/include/nodes/parsenodes.h
*************** typedef struct CreateEnumStmt
*** 2193,2198 ****
--- 2193,2210 ----
List *vals; /* enum values (list of Value strings) */
} CreateEnumStmt;
+ /* ----------------------
+ * Alter Type Statement, enum types
+ * ----------------------
+ */
+ typedef struct AlterEnumStmt
+ {
+ NodeTag type;
+ List *typeName; /* qualified name (list of Value strings) */
+ char *newVal; /* new enum value's name */
+ char *newValNeighbor; /* neighboring enum value, if specified */
+ bool newValIsAfter; /* place new enum value after neighbor? */
+ } AlterEnumStmt;
/* ----------------------
* Create View Statement
diff --git a/src/include/utils/typcache.h b/src/include/utils/typcache.h
index 4065e483e4be6cdfbce1839df2f6ffe19a81aa0c..28b66718b338c9a75276a027b25dc78f7b9fee50 100644
*** a/src/include/utils/typcache.h
--- b/src/include/utils/typcache.h
***************
*** 20,25 ****
--- 20,28 ----
#include "fmgr.h"
+ /* TypeCacheEnumData is an opaque struct known only within typcache.c */
+ struct TypeCacheEnumData;
+
typedef struct TypeCacheEntry
{
/* typeId is the hash lookup key and MUST BE FIRST */
*************** typedef struct TypeCacheEntry
*** 63,68 ****
--- 66,77 ----
* reference-counted tupledesc.)
*/
TupleDesc tupDesc;
+
+ /*
+ * Private information about an enum type. NULL if not enum or
+ * information hasn't been requested.
+ */
+ struct TypeCacheEnumData *enumData;
} TypeCacheEntry;
/* Bit flags to indicate which fields a given caller needs to have set */
*************** extern TupleDesc lookup_rowtype_tupdesc_
*** 86,89 ****
--- 95,100 ----
extern void assign_record_type_typmod(TupleDesc tupDesc);
+ extern int compare_values_of_enum(TypeCacheEntry *tcache, Oid arg1, Oid arg2);
+
#endif /* TYPCACHE_H */
diff --git a/src/test/regress/expected/enum.out b/src/test/regress/expected/enum.out
index 56240c0e7a2966a9dccd2cd9692bc628c52237ab..23389f41485961ba314022368919fa3687583fe2 100644
*** a/src/test/regress/expected/enum.out
--- b/src/test/regress/expected/enum.out
*************** ERROR: invalid input value for enum rai
*** 25,30 ****
--- 25,91 ----
LINE 1: SELECT 'mauve'::rainbow;
^
--
+ -- adding new values
+ --
+ CREATE TYPE planets AS ENUM ( 'venus', 'earth', 'mars' );
+ SELECT enumlabel, enumsortorder
+ FROM pg_enum
+ WHERE enumtypid = 'planets'::regtype
+ ORDER BY 2;
+ enumlabel | enumsortorder
+ -----------+---------------
+ venus | 1
+ earth | 2
+ mars | 3
+ (3 rows)
+
+ ALTER TYPE planets ADD 'uranus';
+ SELECT enumlabel, enumsortorder
+ FROM pg_enum
+ WHERE enumtypid = 'planets'::regtype
+ ORDER BY 2;
+ enumlabel | enumsortorder
+ -----------+---------------
+ venus | 1
+ earth | 2
+ mars | 3
+ uranus | 4
+ (4 rows)
+
+ ALTER TYPE planets ADD 'mercury' BEFORE 'venus';
+ ALTER TYPE planets ADD 'saturn' BEFORE 'uranus';
+ ALTER TYPE planets ADD 'jupiter' AFTER 'mars';
+ ALTER TYPE planets ADD 'neptune' AFTER 'uranus';
+ SELECT enumlabel, enumsortorder
+ FROM pg_enum
+ WHERE enumtypid = 'planets'::regtype
+ ORDER BY 2;
+ enumlabel | enumsortorder
+ -----------+---------------
+ mercury | 0
+ venus | 1
+ earth | 2
+ mars | 3
+ jupiter | 3.25
+ saturn | 3.5
+ uranus | 4
+ neptune | 5
+ (8 rows)
+
+ select 'mars'::planets > 'mercury' as using_sortorder;
+ using_sortorder
+ -----------------
+ t
+ (1 row)
+
+ -- errors for adding labels
+ ALTER TYPE planets ADD
+ 'plutoplutoplutoplutoplutoplutoplutoplutoplutoplutoplutoplutoplutopluto';
+ ERROR: invalid enum label "plutoplutoplutoplutoplutoplutoplutoplutoplutoplutoplutoplutoplutopluto"
+ DETAIL: Labels must be 63 characters or less.
+ ALTER TYPE planets ADD 'pluto' AFTER 'zeus';
+ ERROR: "zeus" is not an existing enum label
+ --
-- Basic table creation, row selection
--
CREATE TABLE enumtest (col rainbow);
*************** SELECT COUNT(*) FROM pg_type WHERE typna
*** 403,409 ****
SELECT * FROM pg_enum WHERE NOT EXISTS
(SELECT 1 FROM pg_type WHERE pg_type.oid = enumtypid);
! enumtypid | enumlabel
! -----------+-----------
(0 rows)
--- 464,470 ----
SELECT * FROM pg_enum WHERE NOT EXISTS
(SELECT 1 FROM pg_type WHERE pg_type.oid = enumtypid);
! enumtypid | enumsortorder | enumlabel
! -----------+---------------+-----------
(0 rows)
diff --git a/src/test/regress/sql/enum.sql b/src/test/regress/sql/enum.sql
index 387e8e72ed8f8e9af85bd7430f9fbfde369a0e3e..53191125be263c06cb91a6bd3184b0549ca9ea83 100644
*** a/src/test/regress/sql/enum.sql
--- b/src/test/regress/sql/enum.sql
*************** SELECT 'red'::rainbow;
*** 16,21 ****
--- 16,57 ----
SELECT 'mauve'::rainbow;
--
+ -- adding new values
+ --
+
+ CREATE TYPE planets AS ENUM ( 'venus', 'earth', 'mars' );
+
+ SELECT enumlabel, enumsortorder
+ FROM pg_enum
+ WHERE enumtypid = 'planets'::regtype
+ ORDER BY 2;
+
+ ALTER TYPE planets ADD 'uranus';
+
+ SELECT enumlabel, enumsortorder
+ FROM pg_enum
+ WHERE enumtypid = 'planets'::regtype
+ ORDER BY 2;
+
+ ALTER TYPE planets ADD 'mercury' BEFORE 'venus';
+ ALTER TYPE planets ADD 'saturn' BEFORE 'uranus';
+ ALTER TYPE planets ADD 'jupiter' AFTER 'mars';
+ ALTER TYPE planets ADD 'neptune' AFTER 'uranus';
+
+ SELECT enumlabel, enumsortorder
+ FROM pg_enum
+ WHERE enumtypid = 'planets'::regtype
+ ORDER BY 2;
+
+ select 'mars'::planets > 'mercury' as using_sortorder;
+
+ -- errors for adding labels
+ ALTER TYPE planets ADD
+ 'plutoplutoplutoplutoplutoplutoplutoplutoplutoplutoplutoplutoplutopluto';
+
+ ALTER TYPE planets ADD 'pluto' AFTER 'zeus';
+
+ --
-- Basic table creation, row selection
--
CREATE TABLE enumtest (col rainbow);
Dean Rasheed <dean.a.rasheed@gmail.com> writes:
> The point with an OID array is that you wouldn't need to store the
> enumsortorder values at all. The sort order would just be the index of
> the OID in the array. So the comparison code would read the OID array,
> traverse it building an array of enum_sort structs {oid, idx}, sort
> that by OID and cache it.
Hmm. But I guess we'd have to keep that array in the pg_type row,
and it'd be a huge PITA to work with at the SQL level. For instance,
psql and pg_dump can easily be adapted to use enumsortorder instead
of pg_enum.oid when they want to read out the labels in sorted order.
Doing the same with an array representation would be a very different
and much uglier query. I'm not eager to contort the catalog
representation that much.
regards, tom lane
<br /><br /> On 10/24/2010 03:33 PM, Tom Lane wrote: <blockquote cite="mid:15783.1287948822@sss.pgh.pa.us"
type="cite"><prewrap="">Dean Rasheed <a class="moz-txt-link-rfc2396E"
href="mailto:dean.a.rasheed@gmail.com"><dean.a.rasheed@gmail.com></a>writes:
</pre><blockquote type="cite"><pre wrap="">The point with an OID array is that you wouldn't need to store the
enumsortorder values at all. The sort order would just be the index of
the OID in the array. So the comparison code would read the OID array,
traverse it building an array of enum_sort structs {oid, idx}, sort
that by OID and cache it.
</pre></blockquote><pre wrap="">
Hmm. But I guess we'd have to keep that array in the pg_type row,
and it'd be a huge PITA to work with at the SQL level. For instance,
psql and pg_dump can easily be adapted to use enumsortorder instead
of pg_enum.oid when they want to read out the labels in sorted order.
Doing the same with an array representation would be a very different
and much uglier query. I'm not eager to contort the catalog
representation that much.
</pre></blockquote><br /> If that's the only objection I don't know that it's terribly serious. psql and pg_dump could
sanelyuse something like:<br /><blockquote>select enum_oid, row_number() over () as sort_order<br /> from
unnest(null::myenum)as enum_oid<br /><br /></blockquote> That said, I'm generally wary of array fields, especially in
thecatalog.<br /><blockquote></blockquote> cheers<br /><br /> andrew<br /><br /><br />
On 10/24/2010 03:28 PM, Tom Lane wrote: > This patch isn't committable as-is because I haven't made enum_first, > enum_last, enum_range follow these coding rules: they need to stop > using the syscache and instead use an indexscan on > pg_enum_typid_sortorder_index to locate the relevant rows. That should > be just a small fix though, and it seems likely to be a net win for > performance anyway. There are a couple of other loose ends too, in > particular I still think we need to prevent ALTER TYPE ADD inside a > transaction block because of the risk of finding undefined enum OIDs > in indexes. Anybody really unhappy with this approach? If not, I'll > finish this up and commit it. I'll look at it tonight. At first glance it looks OK. (BTW, this would be a good case for publishing your development branch somewhere (e.g. git.postgresql.org). That way I could import it and easily do a diff between your patch and mine, in about three simple commands. There are other ways of getting at it, and I'll go and do that, but we should start to look at using the nice facilities git provides.) cheers andrew
<br /><br /> On 10/24/2010 05:09 PM, Andrew Dunstan wrote: <blockquote cite="mid:4CC4A07E.2040104@dunslane.net" type="cite"><br/><br /><blockquote>select enum_oid, row_number() over () as sort_order<br /> from unnest(null::myenum) asenum_oid<br /><br /></blockquote><br /></blockquote><br /> Of course, I meant:<br /><br /><blockquote>select enum_label,row_number() over () as sort_order<br /> from unnest(enum_range(null::myenum)) as enum_label<br /><br /></blockquote>cheers<br /><br /> andrew<br /><blockquote></blockquote>
BTW, I've forgotten --- did anyone publish a test case for checking
performance of enum comparisons? Or should I just cons up something
privately?
regards, tom lane
On 10/24/2010 05:34 PM, Tom Lane wrote: > BTW, I've forgotten --- did anyone publish a test case for checking > performance of enum comparisons? Or should I just cons up something > privately? > I have been running tests with <http://developer.postgresql.org/~adunstan/enumtest.dmp> The table "mydata" contains 100k rows with the column "label" containing random values of an enum type with 500 labels. Basically, my main test is to set up an index on that column. The alter the enum type, and test again. Then alter some of the rows, and test again. cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes: > On 10/24/2010 05:34 PM, Tom Lane wrote: >> BTW, I've forgotten --- did anyone publish a test case for checking >> performance of enum comparisons? Or should I just cons up something >> privately? > I have been running tests with > <http://developer.postgresql.org/~adunstan/enumtest.dmp> > The table "mydata" contains 100k rows with the column "label" containing > random values of an enum type with 500 labels. Basically, my main test > is to set up an index on that column. The alter the enum type, and test > again. Then alter some of the rows, and test again. OK, I did some timing consisting of building a btree index with maintenance_work_mem set reasonably high. This is on a debug-enabled build, so it's not representative of production performance, but it will do for seeing what we're doing to enum comparison performance. Here's what I tried: Stock 9.0.1 24.9 sec patch, all OIDs even 25.2 sec (~ 1% hit) patch, half of OIDs odd 27.2 sec (~ 9% hit) same, bitmapset forced null 64.9 sec (~ 160% hit) (Note that the noise level in these measurements is about 1%; I'm not entirely convinced that the all-even case is really measurably slower than 9.0.) The "half of OIDs odd" case is what you'd get for a binary upgrade from a 9.0 database. The last case shows what happens if the intermediate bitmapset-test optimization is disabled, forcing all comparisons to do binary searches in the sorted-by-OID array (except for the one-quarter of cases where both OIDs are even by chance). It's pretty grim but it represents a worst case that you'd be very unlikely to hit in practice. This shows that the bitmapset optimization really is quite effective, at least for cases where all the enum labels are sorted by OID after all. That motivated me to change the bitmapset setup code to what's attached. This is potentially a little slower at initializing the cache, but it makes up for that by still marking most enum members as sorted even when a few out-of-order members have been inserted. The key point is that an out-of-order member in the middle of the array doesn't prevent us from considering following members as properly sorted, as long as they are correctly ordered with respect to the other properly-sorted members. With this approach we can honestly say that inserting an out-of-order enum value doesn't impact comparison performance for pre-existing enum members, only for comparisons involving the out-of-order value itself; even when the existing members were binary-upgraded and thus weren't all even. I think that's a worthwhile advantage. IMHO this level of performance is good enough. Anyone unhappy? regards, tom lane /* * Here, we create a bitmap listing a subset of the enum's OIDs that are * known to be in order and can thus becompared with just OID comparison. * * The point of this is that the enum's initial OIDs were certainly in * order,so there is some subset that can be compared via OID comparison; * and we'd rather not do binary searches unnecessarily. * * This is somewhat heuristic, and might identify a subset of OIDs that * isn't exactly what thetype started with. That's okay as long as * the subset is correctly sorted. */ bitmap_base = InvalidOid; bitmap= NULL; bm_size = 1; /* only save sets of at least 2 OIDs */ for (start_pos = 0; start_pos < numitems - 1; start_pos++) { /* * Identify longest sorted subsequence startingat start_pos */ Bitmapset *this_bitmap = bms_make_singleton(0); int this_bm_size = 1; Oid start_oid = items[start_pos].enum_oid; float4 prev_order = items[start_pos].sort_order; int i; for (i = start_pos + 1; i < numitems; i++) { Oid offset; offset = items[i].enum_oid - start_oid; /* quit if bitmap would be too large; cutoff is arbitrary */ if (offset >= 1024) break; /* include the item if it's in-order */ if (items[i].sort_order> prev_order) { prev_order = items[i].sort_order; this_bitmap =bms_add_member(this_bitmap, (int) offset); this_bm_size++; } } /* Remember it if larger than previous best */ if (this_bm_size > bm_size) { bms_free(bitmap); bitmap_base = start_oid; bitmap = this_bitmap; bm_size = this_bm_size; } else bms_free(this_bitmap); /* * Done if it's not possible to find a longer sequence in the rest * of the list. In typical casesthis will happen on the first * iteration, which is why we create the bitmaps on the fly instead * ofdoing a second pass over the list. */ if (bm_size >= (numitems - start_pos - 1)) break; }
On 10/24/2010 08:12 PM, Tom Lane wrote: > OK, I did some timing consisting of building a btree index with > maintenance_work_mem set reasonably high. This is on a debug-enabled > build, so it's not representative of production performance, but it will > do for seeing what we're doing to enum comparison performance. Here's > what I tried: > > Stock 9.0.1 24.9 sec > > patch, all OIDs even 25.2 sec (~ 1% hit) > > patch, half of OIDs odd 27.2 sec (~ 9% hit) > > same, bitmapset forced null 64.9 sec (~ 160% hit) > > (Note that the noise level in these measurements is about 1%; > I'm not entirely convinced that the all-even case is really measurably > slower than 9.0.) Yeah, that was my conclusion. I tested with debug/cassert turned off, but my results were similar. > The "half of OIDs odd" case is what you'd get for a binary upgrade > from a 9.0 database. The last case shows what happens if the > intermediate bitmapset-test optimization is disabled, forcing all > comparisons to do binary searches in the sorted-by-OID array > (except for the one-quarter of cases where both OIDs are even > by chance). It's pretty grim but it represents a worst case that > you'd be very unlikely to hit in practice. > > This shows that the bitmapset optimization really is quite effective, > at least for cases where all the enum labels are sorted by OID after > all. That motivated me to change the bitmapset setup code to what's > attached. This is potentially a little slower at initializing the > cache, but it makes up for that by still marking most enum members > as sorted even when a few out-of-order members have been inserted. > The key point is that an out-of-order member in the middle of the > array doesn't prevent us from considering following members as > properly sorted, as long as they are correctly ordered with respect to > the other properly-sorted members. That's nice. It's a tradeoff though. Bumping up the cost of setting up the cache won't have much effect on a creating a large index, but could affect to performance of retail comparisons significantly. But this is probably worth it. You'd have to work hard to create the perverse case that could result in seriously worse cache setup cost. > With this approach we can honestly say that inserting an out-of-order > enum value doesn't impact comparison performance for pre-existing > enum members, only for comparisons involving the out-of-order value > itself; even when the existing members were binary-upgraded and thus > weren't all even. I think that's a worthwhile advantage. Yeah, that's nice. > IMHO this level of performance is good enough. Anyone unhappy? No, seems good. cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> On 10/24/2010 08:12 PM, Tom Lane wrote:
>> This shows that the bitmapset optimization really is quite effective,
>> at least for cases where all the enum labels are sorted by OID after
>> all. That motivated me to change the bitmapset setup code to what's
>> attached. This is potentially a little slower at initializing the
>> cache, but it makes up for that by still marking most enum members
>> as sorted even when a few out-of-order members have been inserted.
> That's nice. It's a tradeoff though. Bumping up the cost of setting up
> the cache won't have much effect on a creating a large index, but could
> affect to performance of retail comparisons significantly. But this is
> probably worth it. You'd have to work hard to create the perverse case
> that could result in seriously worse cache setup cost.
Well, notice that I moved the caching into typcache.c, rather than
having it be associated with query startup. So unless you're actively
frobbing the enum definition, that's going to be paid only once per
session.
regards, tom lane
On 10/24/2010 09:20 PM, Tom Lane wrote: > Andrew Dunstan<andrew@dunslane.net> writes: >> On 10/24/2010 08:12 PM, Tom Lane wrote: >>> This shows that the bitmapset optimization really is quite effective, >>> at least for cases where all the enum labels are sorted by OID after >>> all. That motivated me to change the bitmapset setup code to what's >>> attached. This is potentially a little slower at initializing the >>> cache, but it makes up for that by still marking most enum members >>> as sorted even when a few out-of-order members have been inserted. >> That's nice. It's a tradeoff though. Bumping up the cost of setting up >> the cache won't have much effect on a creating a large index, but could >> affect to performance of retail comparisons significantly. But this is >> probably worth it. You'd have to work hard to create the perverse case >> that could result in seriously worse cache setup cost. > Well, notice that I moved the caching into typcache.c, rather than > having it be associated with query startup. So unless you're actively > frobbing the enum definition, that's going to be paid only once per > session. Oh, yes. Good. I'm just starting to look at this in detail. cheers andrew
Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> writes: > > On 10/24/2010 08:12 PM, Tom Lane wrote: > >> This shows that the bitmapset optimization really is quite effective, > >> at least for cases where all the enum labels are sorted by OID after > >> all. That motivated me to change the bitmapset setup code to what's > >> attached. This is potentially a little slower at initializing the > >> cache, but it makes up for that by still marking most enum members > >> as sorted even when a few out-of-order members have been inserted. > > > That's nice. It's a tradeoff though. Bumping up the cost of setting up > > the cache won't have much effect on a creating a large index, but could > > affect to performance of retail comparisons significantly. But this is > > probably worth it. You'd have to work hard to create the perverse case > > that could result in seriously worse cache setup cost. > > Well, notice that I moved the caching into typcache.c, rather than > having it be associated with query startup. So unless you're actively > frobbing the enum definition, that's going to be paid only once per > session. Thanks for modifying pg_upgrade so it works with this new format. The changes look good and cleaner than what I had to do for 9.0. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> writes: > > On 10/24/2010 08:12 PM, Tom Lane wrote: > >> This shows that the bitmapset optimization really is quite effective, > >> at least for cases where all the enum labels are sorted by OID after > >> all. That motivated me to change the bitmapset setup code to what's > >> attached. This is potentially a little slower at initializing the > >> cache, but it makes up for that by still marking most enum members > >> as sorted even when a few out-of-order members have been inserted. > > > That's nice. It's a tradeoff though. Bumping up the cost of setting up > > the cache won't have much effect on a creating a large index, but could > > affect to performance of retail comparisons significantly. But this is > > probably worth it. You'd have to work hard to create the perverse case > > that could result in seriously worse cache setup cost. > > Well, notice that I moved the caching into typcache.c, rather than > having it be associated with query startup. So unless you're actively > frobbing the enum definition, that's going to be paid only once per > session. FYI, I marked the TODO item for adding enums as completed. The TODO item used to also mention renaming or removing enums, but I have seen few requests for that so I removed that suggestion. We can always re-add it if there is demand. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
On 11/12/2010 01:40 PM, Bruce Momjian wrote: > FYI, I marked the TODO item for adding enums as completed. The TODO > item used to also mention renaming or removing enums, but I have seen > few requests for that so I removed that suggestion. We can always > re-add it if there is demand. Renaming an item would not be terribly hard. Removing one is that nasty case. There are all sorts of places the old value could be referred to: table data, view definitions, check constraints, functions etc. cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> On 11/12/2010 01:40 PM, Bruce Momjian wrote:
>> FYI, I marked the TODO item for adding enums as completed. The TODO
>> item used to also mention renaming or removing enums, but I have seen
>> few requests for that so I removed that suggestion. We can always
>> re-add it if there is demand.
> Renaming an item would not be terribly hard. Removing one is that nasty
> case. There are all sorts of places the old value could be referred to:
> table data, view definitions, check constraints, functions etc.
Well, you can rename an item today if you don't mind doing a direct
UPDATE on pg_enum. I think that's probably sufficient if the demand
only amounts to one or two requests a year. I'd say leave it off the
TODO list till we see if there's more demand than that.
regards, tom lane
On Fri, Nov 12, 2010 at 1:55 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Andrew Dunstan <andrew@dunslane.net> writes: >> On 11/12/2010 01:40 PM, Bruce Momjian wrote: >>> FYI, I marked the TODO item for adding enums as completed. The TODO >>> item used to also mention renaming or removing enums, but I have seen >>> few requests for that so I removed that suggestion. We can always >>> re-add it if there is demand. > >> Renaming an item would not be terribly hard. Removing one is that nasty >> case. There are all sorts of places the old value could be referred to: >> table data, view definitions, check constraints, functions etc. > > Well, you can rename an item today if you don't mind doing a direct > UPDATE on pg_enum. I think that's probably sufficient if the demand > only amounts to one or two requests a year. I'd say leave it off the > TODO list till we see if there's more demand than that. I'd say put it on and mark it with an [E]. We could use some more [E]asy items for that list. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Fri, Nov 12, 2010 at 1:55 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Well, you can rename an item today if you don't mind doing a direct
>> UPDATE on pg_enum. �I think that's probably sufficient if the demand
>> only amounts to one or two requests a year. �I'd say leave it off the
>> TODO list till we see if there's more demand than that.
> I'd say put it on and mark it with an [E]. We could use some more
> [E]asy items for that list.
We don't need to add marginally-useful features just because they're
easy. If it doesn't have a real use-case, the incremental maintenance
cost of more code is a good reason to reject it.
regards, tom lane
Excerpts from Bruce Momjian's message of vie nov 12 15:40:28 -0300 2010: > FYI, I marked the TODO item for adding enums as completed. The TODO > item used to also mention renaming or removing enums, but I have seen > few requests for that so I removed that suggestion. We can always > re-add it if there is demand. I'm sure there's going to be more demand for ENUM features, now that they are more usable. -- Álvaro Herrera <alvherre@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
On Nov 12, 2010, at 2:20 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Fri, Nov 12, 2010 at 1:55 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> Well, you can rename an item today if you don't mind doing a direct >>> UPDATE on pg_enum. I think that's probably sufficient if the demand >>> only amounts to one or two requests a year. I'd say leave it off the >>> TODO list till we see if there's more demand than that. > >> I'd say put it on and mark it with an [E]. We could use some more >> [E]asy items for that list. > > We don't need to add marginally-useful features just because they're > easy. If it doesn't have a real use-case, the incremental maintenance > cost of more code is a good reason to reject it. If we allow users to name objects, we ought to make every effort to also allow renaming them. In my mind, the only way renamingis too marginal to be useful is if the feature itself is too marginal to be useful. ...Robert
On Fri, 2010-11-12 at 14:20 -0500, Tom Lane wrote: > Robert Haas <robertmhaas@gmail.com> writes: > > On Fri, Nov 12, 2010 at 1:55 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > >> Well, you can rename an item today if you don't mind doing a direct > >> UPDATE on pg_enum. I think that's probably sufficient if the demand > >> only amounts to one or two requests a year. I'd say leave it off the > >> TODO list till we see if there's more demand than that. > > > I'd say put it on and mark it with an [E]. We could use some more > > [E]asy items for that list. > > We don't need to add marginally-useful features just because they're > easy. If it doesn't have a real use-case, the incremental maintenance > cost of more code is a good reason to reject it. Perhaps we should remove the ability to rename tables and databases too. It would certainly lighten the code path. JD > > regards, tom lane > -- PostgreSQL.org Major Contributor Command Prompt, Inc: http://www.commandprompt.com/ - 509.416.6579 Consulting, Training, Support, Custom Development, Engineering http://twitter.com/cmdpromptinc | http://identi.ca/commandprompt
Joshua D. Drake wrote: > On Fri, 2010-11-12 at 14:20 -0500, Tom Lane wrote: > > Robert Haas <robertmhaas@gmail.com> writes: > > > On Fri, Nov 12, 2010 at 1:55 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > >> Well, you can rename an item today if you don't mind doing a direct > > >> UPDATE on pg_enum. I think that's probably sufficient if the demand > > >> only amounts to one or two requests a year. I'd say leave it off the > > >> TODO list till we see if there's more demand than that. > > > > > I'd say put it on and mark it with an [E]. We could use some more > > > [E]asy items for that list. > > > > We don't need to add marginally-useful features just because they're > > easy. If it doesn't have a real use-case, the incremental maintenance > > cost of more code is a good reason to reject it. > > Perhaps we should remove the ability to rename tables and databases too. > It would certainly lighten the code path. OK, got it. Added incomplete TODO item: Allow renaming and deleting enumerated values from an existingenumerated data type -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
On 11/12/2010 09:18 PM, Bruce Momjian wrote: > OK, got it. Added incomplete TODO item: > > Allow renaming and deleting enumerated values from an existing > enumerated data type > I have serious doubts that deleting will ever be sanely doable. cheers andrew
Andrew Dunstan wrote: > > > On 11/12/2010 09:18 PM, Bruce Momjian wrote: > > OK, got it. Added incomplete TODO item: > > > > Allow renaming and deleting enumerated values from an existing > > enumerated data type > > > > > I have serious doubts that deleting will ever be sanely doable. True. Should we not mention it then? I can't think of many objects we can't delete. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
On fre, 2010-11-12 at 17:19 -0500, Robert Haas wrote: > If we allow users to name objects, we ought to make every effort to > also allow renaming them. In my mind, the only way renaming is too > marginal to be useful is if the feature itself is too marginal to be > useful. The bottom line is, any kind of database object needs to be changeable and removable, otherwise there will always be hesitations about its use. And when there are hesitations about the use, it's often easiest not to bother. I remember ten years ago or so we used to send people away who requested the ability to drop columns, claiming they didn't plan their database properly, or they should load it from scratch. Nowadays that is ludicrous; databases live forever, development is agile, everything needs to be changeable.
On Sat, Nov 13, 2010 at 12:30 PM, Peter Eisentraut <peter_e@gmx.net> wrote: > On fre, 2010-11-12 at 17:19 -0500, Robert Haas wrote: >> If we allow users to name objects, we ought to make every effort to >> also allow renaming them. In my mind, the only way renaming is too >> marginal to be useful is if the feature itself is too marginal to be >> useful. > > The bottom line is, any kind of database object needs to be changeable > and removable, otherwise there will always be hesitations about its use. > And when there are hesitations about the use, it's often easiest not to > bother. > > I remember ten years ago or so we used to send people away who requested > the ability to drop columns, claiming they didn't plan their database > properly, or they should load it from scratch. Nowadays that is > ludicrous; databases live forever, development is agile, everything > needs to be changeable. It was ludicrous then, too. I picked MySQL for several projects early on for precisely the lack of the ability to drop columns in PostgreSQL. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
>> I'd say put it on and mark it with an [E]. We could use some more
>> [E]asy items for that list.
>
> We don't need to add marginally-useful features just because they're
> easy. If it doesn't have a real use-case, the incremental maintenance
> cost of more code is a good reason to reject it.
I'll bite.
Use-case:
1) DBA adds "Department Role" enum, with set
{'Director','Secretary','Staff','Support','Temporary','Liason'}.
2) 3-person data entry team updates all employee records with those roles.
3) First summary report is run.
4) Manager points out that "Liason" is misspelled.
-- -- Josh Berkus PostgreSQL Experts Inc.
http://www.pgexperts.com