Re: [PATCH] Alter or rename enum value

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
guide.

>> + /* 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.