Re: ALTER TABLE does not check for column existence before starting operations

From: Arthur Zakirov <a(dot)zakirov(at)postgrespro(dot)ru>
To: Pierre Ducroquet <pierre(dot)ducroquet(at)people-doc(dot)com>
Cc: PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: ALTER TABLE does not check for column existence before starting operations
Date: 2018-06-26 15:59:18
Message-ID: 20180626155917.GA10612@zakirov.localdomain
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

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

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Pavel Stehule 2018-06-26 16:07:01 Re: Unexpected behavior of DROP VIEW/TABLE IF EXISTS
Previous Message Fujii Masao 2018-06-26 15:58:18 Re: Small fixes about backup history file in doc and pg_standby