Обсуждение: ALTER TABLE does not check for column existence before starting operations

Hi

While working on a big table recently, I noticed that ALTER TABLE does not 
check for column existence in operations like SET NOT NULL before starting 
working on the table, for instance adding a primary key.
It is thus possible, if a typo has been made, to generate a long lock and a 
lot of WAL that will serve no purpose since the whole transaction will be 
discarded.

For example :

toto=# alter table test add primary key(i), alter column typo set not null;
ERROR:  column "typo" of relation "test" does not exist
Time: 10.794 s


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.


Regards

 Pierre

Hi Pierre,

On 3/2/18 6:36 AM, Pierre Ducroquet wrote:
> 
> While working on a big table recently, I noticed that ALTER TABLE does not 
> check for column existence in operations like SET NOT NULL before starting 
> working on the table, for instance adding a primary key.
> It is thus possible, if a typo has been made, to generate a long lock and a 
> lot of WAL that will serve no purpose since the whole transaction will be 
> discarded.
> 
> For example :
> 
> toto=# alter table test add primary key(i), alter column typo set not null;
> ERROR:  column "typo" of relation "test" does not exist
> Time: 10.794 s
> 
> 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.

This looks like a good idea.  However, the last CF for PG11 is in
progress so it might be difficult to attract much comment/review right now.

I recommend entering this patch in the 2018-09 CF so it doesn't get lost.

Regards,
-- 
-David
david@pgmasters.net

On Friday, March 2, 2018 2:44:16 PM CET David Steele wrote:
> Hi Pierre,
>
> On 3/2/18 6:36 AM, Pierre Ducroquet wrote:
> > While working on a big table recently, I noticed that ALTER TABLE does not
> > check for column existence in operations like SET NOT NULL before starting
> > working on the table, for instance adding a primary key.
> > It is thus possible, if a typo has been made, to generate a long lock and
> > a
> > lot of WAL that will serve no purpose since the whole transaction will be
> > discarded.
> >
> > For example :
> >
> > toto=# alter table test add primary key(i), alter column typo set not
> > null;
> > ERROR:  column "typo" of relation "test" does not exist
> > Time: 10.794 s
> >
> > 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.
> This looks like a good idea.  However, the last CF for PG11 is in
> progress so it might be difficult to attract much comment/review right now.
>
> I recommend entering this patch in the 2018-09 CF so it doesn't get lost.

Hi

Thanks for the answer.
I saw that bug two days ago but I had no time then to do the patch. Had I seen
the CF window was that close I would have hurried up… Heh, this will just wait
a few months. I will enter it in the 2018-09 CF as soon as it opens.

Regards

 Pierre

Pierre Ducroquet <pierre.ducroquet@people-doc.com> writes:
> On Friday, March 2, 2018 2:44:16 PM CET David Steele wrote:
>> I recommend entering this patch in the 2018-09 CF so it doesn't get lost.

> Thanks for the answer.
> I saw that bug two days ago but I had no time then to do the patch. Had I seen
> the CF window was that close I would have hurried up… Heh, this will just wait
> a few months. I will enter it in the 2018-09 CF as soon as it opens.

You can add entries to the -09 CF now.

            regards, tom lane

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