On Tue, Jan 3, 2012 at 10:38 AM, Pavel Stehule <pavel.steh...@gmail.com> wrote: > Hello > > 2012/1/3 Robert Haas <robertmh...@gmail.com>: >> On Mon, Jan 2, 2012 at 12:01 PM, Pavel Stehule <pavel.steh...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers