Re: [PATCH] Alter or rename enum value

From: ilmari(at)ilmari(dot)org (Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?=)
To: emre(at)hasegeli(dot)com
Cc: PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Andrew Dunstan <andrew(at)dunslane(dot)net>, Matthias Kurz <m(dot)kurz(at)irregular(dot)at>, Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)com>, "David G(dot) Johnston" <david(dot)g(dot)johnston(at)gmail(dot)com>
Subject: Re: [PATCH] Alter or rename enum value
Date: 2016-08-24 12:11:43
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Emre Hasegeli <emre(at)hasegeli(dot)com> writes:

>> Here is v4, which changes the command from ALTER VALUE to RENAME VALUE,
>> for consistency with RENAME ATTRIBUTE.
> It looks like we always use "ALTER ... RENAME ... old_name TO
> new_name" syntax, so it is better that way. I have noticed that all
> the other RENAMEs go through ExecRenameStmt(). We better do the same.

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.

>> + int nbr_index;
>> + Form_pg_enum nbr_en;
> 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

>> + /* Locate the element to rename and check if the new label is already in
> The project style for multi-line commands is to leave the first line
> of the comment block empty.
>> + if (strcmp(NameStr(nbr_en->enumlabel), oldVal) == 0)
> I found namestrcmp() for this.

Thanks, fixed. This again came from using AddEnumLabel() as an example,
which should probably be fixed separately.

>> + }
>> + if (!old_tup)
> I would leave a space in between.
>> + ReleaseCatCacheList(list);
>> + heap_close(pg_enum, RowExclusiveLock);
> 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.

Here is an updated patch.

Attachment Content-Type Size
0001-Add-ALTER-TYPE-RENAME-VALUE-TO-v5.patch text/x-diff 17.7 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Peter Eisentraut 2016-08-24 12:40:04 Re: Showing parallel status in \df+
Previous Message Thomas Munro 2016-08-24 12:04:40 Re: [RFC] Change the default of update_process_title to off