Re: patch: ALTER TABLE IF EXISTS

On Tue, Jan 3, 2012 at 10:38 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> Hello
>
> 2012/1/3 Robert Haas <robertmhaas@gmail.com>:
>> On Mon, Jan 2, 2012 at 12:01 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
>>> here is updated patch
>>
>> I think the comments in parse_utilcmd.c probably need a bit of adjustment.
>
> I don't see it - there is only one comment and it is adjusted with
> "if" statement.
>
> please, show it

Well, basically, the comment preceding the part you altered say "the
lock level requested here", but "here" is getting spread out quite a
bit more with this code change.  Maybe that doesn't matter.

However, on further examination, this is a pretty awkward way to write
the code.  Why not something like this:

rel = relation_openrv_extended(stmt->relation, lockmode, stmt->missing_ok);
if (rel == NULL)
{   ereport(...);   return NIL;
}

Maybe the intent of sticking heap_openrv_extended() into the upper
branch of the if statement is to try to bounce relations that aren't
tables, but that's not actually what that function does (e.g. a
foreign table will slip through).  And I think if we're going to have
IF EXISTS support for ALTER TABLE at all, we ought to have it for the
whole family of related statements: ALTER VIEW, ALTER SEQUENCE, ALTER
FOREIGN TABLE, etc., not just ALTER TABLE itself.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company