Re: [PATCH] Alter or rename enum value
Emre Hasegeli <emre@hasegeli.com>
From: Emre Hasegeli <emre@hasegeli.com>
To: Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>
Cc: PostgreSQL Hackers <pgsql-hackers@postgresql.org>,
Tom Lane <tgl@sss.pgh.pa.us>, Andrew Dunstan <andrew@dunslane.net>, Matthias Kurz <m.kurz@irregular.at>, Jim Nasby <Jim.Nasby@bluetreble.com>,
"David G. Johnston" <david.g.johnston@gmail.com>
Date: 2016-08-24T14:47:19Z
Lists: pgsql-hackers
> That would require changing it from an AlterEnumStmt to a RenameStmt > instead. Those look to me like they're for renaming SQL objects, > i.e. things addressed by identifiers, whereas enum labels are just > strings. Looking at the implementation of a few of the functions called > by ExecRenameStmt(), they have very little in common with > RenameEnumLabel() logic-wise. Yes, you are right that this is not an identifier like others. On the other hand, all RENAME xxx TO yyy statements are RenameStmt at the moment. I think we better leave the decision to the committer. >> What is "nbr"? Why not calling them something like "i" and "val"? > > This is the same naming as used in AddEnumLabel(), which I used as a > guide. I see. Judging from there, it should be shortcut for neighbour. We better use something else. >> Maybe we better release them before reporting error, too. I would >> release the list after the loop, close the heap before ereport(). > > As Tom said, this gets released automatically on error, and is again > similar to how AddEnumLabel() does it. I still don't see any reason not to ReleaseCatCacheList() after the loop instead of writing it 3 times. > Here is an updated patch. I don't know, if it is used by the committer or not, but "existance" is a typo on the commit message. Other than these, it looks good to me. I am marking it as Ready for Committer.
Commits
-
Relax transactional restrictions on ALTER TYPE ... ADD VALUE (redux).
- 212fab9926b2 12.0 landed
-
Partially restore comments discussing enum renumbering hazards.
- c9e2e2db5c20 9.4.0 cited
-
Allow adding values to an enum type created in the current transaction.
- 7b90469b7176 9.3.0 cited