Re: [PATCH] Alter or rename enum value

From: Emre Hasegeli <emre(at)hasegeli(dot)com>
To: Dagfinn Ilmari Mannsåker <ilmari(at)ilmari(dot)org>
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 14:47:19
Message-ID: CAE2gYzy9fWFmMgnA0DstcGdtuFv40d3JXgtikE-ywtZ2qow=ng@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
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.

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2016-08-24 15:08:28 Re: Strange result with LATERAL query
Previous Message Tom Lane 2016-08-24 14:24:22 Re: Showing parallel status in \df+