Re: ALTER TABLE does not check for column existence before startingoperations

Hello,

On Fri, Mar 02, 2018 at 12:36:41PM +0100, Pierre Ducroquet wrote:
> The attached patch fixes this behaviour by adding a small check in the first 
> pass of alter table to make sure that a column referenced by an alter command 
> exists first. It also checks if the column is added by another alter sub-
> command. It does not handle every scenario (dropping a column and then 
> altering it for instance), these are left to the exec code to exclude.
> The patch has been checked with make check, and I see no documentation change 
> to do since this does not alter any existing documented behaviour.

I looked at the patch. It is true that there is no need to change the
documentation. Tests also passes (but maybe some changes would be
needed).

I have a couple comments:

> tuple = SearchSysCacheCopyAttName(RelationGetRelid(rel), colName);

I think it is necessary to release the heap tuple using heap_freetuple()
if it is valid after all work done.

Second comment is related with several subcommands (ALTER COLUMN
DEFAULT, SET NOT NULL, SET/RESET (options)). The following query fails
with the patch:

=# alter table test
   alter non_existing set not null,
   add non_existing text;

It raises the error 'column "non_existing" of relation "test" does not
exist'. But without the patch the query is executed without errors. It
is because of how Phase 2 is performed. Subcommands are executed in a pass
determined by subcommand type. AT_PASS_ADD_COL subcommands are executed
before AT_PASS_ADD_INDEX, AT_PASS_ADD_CONSTR and AT_PASS_MISC.

I'm not sure how important it is. But I think it could break backward
compatibility.

-- 
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company