Обсуждение: BUG #3403: ver 8.2 can't add serial column to temp table,but 8.1 can

The following bug has been logged online:

Bug reference:      3403
Logged by:          Jasen Betts
Email address:      jasen@treshna.com
PostgreSQL version: 8.2.0
Operating system:   window XP (vmware)
Description:        ver 8.2 can't add serial column to temp table,but 8.1
can
Details:

gymmaster=# select version();
                                         version

--------
----------------------------------------------------------------------------
------
 PostgreSQL 8.2.0 on i686-pc-mingw32, compiled by GCC cc.exe (GCC) 3.4.2
(mingw-special)
(1 row)
template1=# create temp table foo ( x text);
CREATE TABLE
template1=# alter table foo add column y text ;
ALTER TABLE
template1=# alter table foo add column id serial;
NOTICE:  ALTER TABLE will create implicit sequence "foo_id_seq" for serial
colum
n "foo.id"
ERROR:  relation "public.foo" does not exist
template1=#


this worked in version 8.1.8 (linux)

Jasen Betts wrote:
> The following bug has been logged online:
>
> Bug reference:      3403
> Logged by:          Jasen Betts
> Email address:      jasen@treshna.com
> PostgreSQL version: 8.2.0
> Operating system:   window XP (vmware)
> Description:        ver 8.2 can't add serial column to temp table,but 8.1
> can
> Details:
>
> gymmaster=# select version();
>                                          version
>
> --------
> ----------------------------------------------------------------------------
> ------
>  PostgreSQL 8.2.0 on i686-pc-mingw32, compiled by GCC cc.exe (GCC) 3.4.2
> (mingw-special)
> (1 row)
> template1=# create temp table foo ( x text);
> CREATE TABLE
> template1=# alter table foo add column y text ;
> ALTER TABLE
> template1=# alter table foo add column id serial;
> NOTICE:  ALTER TABLE will create implicit sequence "foo_id_seq" for serial
> colum
> n "foo.id"
> ERROR:  relation "public.foo" does not exist
> template1=#

It does not work on 8.2.4 as well. It seems PG lost information about
schema and try to use default schema. Following command works well:

alter table pg_temp.foo add column id serial;

It could be use as workaround.


        Zdenek

Zdenek Kotala wrote:
> Jasen Betts wrote:
>> template1=# create temp table foo ( x text);
>> CREATE TABLE
>> template1=# alter table foo add column y text ;
>> ALTER TABLE
>> template1=# alter table foo add column id serial;
>> NOTICE:  ALTER TABLE will create implicit sequence "foo_id_seq" for
>> serial
>> colum
>> n "foo.id"
>> ERROR:  relation "public.foo" does not exist
>> template1=#
>
> It does not work on 8.2.4 as well. It seems PG lost information about
> schema and try to use default schema. Following command works well:
>
> alter table pg_temp.foo add column id serial;
>
> It could be use as workaround.

8.1 creates the sequence in wrong schema:

postgres=# create temp table foo ( x text);
CREATE TABLE
postgres=# alter table foo add column id serial;
NOTICE:  ALTER TABLE will create implicit sequence "foo_id_seq" for
serial column "foo.id"
ALTER TABLE
postgres=# \d
               List of relations
   Schema   |    Name    |   Type   |  Owner
-----------+------------+----------+----------
  pg_temp_1 | foo        | table    | hlinnaka
  public    | foo_id_seq | sequence | hlinnaka
(2 rows)

The problem seems to be in transformColumnDefinition, where the schema
of the to-be-created sequence is determined from the relation name
given. The default creation schema is used, if the user didn't specify
the schame of the table explicitly, but since it's an ALTER TABLE, it
really should use the schema of the existing table.

Patch against 8.2 attached, seems to apply to 8.1 and CVS head though I
haven't tested them.. This is not my area of expertise, so I'm not 100%
sure this is the right way to fix it.

--
   Heikki Linnakangas
   EnterpriseDB   http://www.enterprisedb.com
Index: src/backend/parser/analyze.c
===================================================================
RCS file: /home/hlinnaka/pgcvsrepository/pgsql/src/backend/parser/analyze.c,v
retrieving revision 1.353
diff -c -r1.353 analyze.c
*** src/backend/parser/analyze.c    11 Oct 2006 16:42:59 -0000    1.353
--- src/backend/parser/analyze.c    22 Jun 2007 10:16:20 -0000
***************
*** 69,74 ****
--- 69,75 ----
  {
      const char *stmtType;        /* "CREATE TABLE" or "ALTER TABLE" */
      RangeVar   *relation;        /* relation to create */
+     Oid            namespace;        /* namespace of relation to alter */
      List       *inhRelations;    /* relations to inherit from */
      bool        hasoids;        /* does relation have an OID column? */
      bool        isalter;        /* true if altering existing table */
***************
*** 945,950 ****
--- 946,952 ----

      cxt.stmtType = "CREATE TABLE";
      cxt.relation = stmt->relation;
+     cxt.namespace = InvalidOid;
      cxt.inhRelations = stmt->inhRelations;
      cxt.isalter = false;
      cxt.columns = NIL;
***************
*** 1087,1093 ****
           * quite unlikely to be a problem, especially since few people would
           * need two serial columns in one table.
           */
!         snamespaceid = RangeVarGetCreationNamespace(cxt->relation);
          snamespace = get_namespace_name(snamespaceid);
          sname = ChooseRelationName(cxt->relation->relname,
                                     column->colname,
--- 1089,1098 ----
           * quite unlikely to be a problem, especially since few people would
           * need two serial columns in one table.
           */
!         if (OidIsValid(cxt->namespace))
!            snamespaceid = cxt->namespace;
!         else
!            snamespaceid = RangeVarGetCreationNamespace(cxt->relation);
          snamespace = get_namespace_name(snamespaceid);
          sname = ChooseRelationName(cxt->relation->relname,
                                     column->colname,
***************
*** 3010,3015 ****
--- 3015,3021 ----
      List       *newcmds = NIL;
      bool        skipValidation = true;
      AlterTableCmd *newcmd;
+     Relation    relation;

      cxt.stmtType = "ALTER TABLE";
      cxt.relation = stmt->relation;
***************
*** 3024,3029 ****
--- 3030,3045 ----
      cxt.alist = NIL;
      cxt.pkey = NULL;

+     relation = heap_openrv(stmt->relation, AccessShareLock);
+     if (relation->rd_rel->relkind != RELKIND_RELATION)
+         ereport(ERROR,
+                 (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+                  errmsg("relation \"%s\" is not a table",
+                         stmt->relation->relname)));
+
+     cxt.namespace = RelationGetNamespace(relation);
+
+
      /*
       * The only subtypes that currently require parse transformation handling
       * are ADD COLUMN and ADD CONSTRAINT.  These largely re-use code from
***************
*** 3166,3171 ****
--- 3182,3194 ----
      *extras_before = list_concat(*extras_before, cxt.blist);
      *extras_after = list_concat(cxt.alist, *extras_after);

+     /*
+      * Close the parent rel, but keep our AccessShareLock on it until xact
+      * commit.    That will prevent someone else from deleting or ALTERing the
+      * table before we get to execute the changes.
+      */
+     heap_close(relation, NoLock);
+
      return qry;
  }

Heikki Linnakangas wrote:
> Zdenek Kotala wrote:
>> Jasen Betts wrote:
>>> template1=# create temp table foo ( x text);
>>> CREATE TABLE
>>> template1=# alter table foo add column y text ;
>>> ALTER TABLE
>>> template1=# alter table foo add column id serial;
>>> NOTICE:  ALTER TABLE will create implicit sequence "foo_id_seq" for
>>> serial
>>> colum
>>> n "foo.id"
>>> ERROR:  relation "public.foo" does not exist
>>> template1=#
>>
>> It does not work on 8.2.4 as well. It seems PG lost information about
>> schema and try to use default schema. Following command works well:
>>
>> alter table pg_temp.foo add column id serial;
>>
>> It could be use as workaround.
>
> 8.1 creates the sequence in wrong schema:
>
> postgres=# create temp table foo ( x text);
> CREATE TABLE
> postgres=# alter table foo add column id serial;
> NOTICE:  ALTER TABLE will create implicit sequence "foo_id_seq" for
> serial column "foo.id"
> ALTER TABLE
> postgres=# \d
>               List of relations
>   Schema   |    Name    |   Type   |  Owner
> -----------+------------+----------+----------
>  pg_temp_1 | foo        | table    | hlinnaka
>  public    | foo_id_seq | sequence | hlinnaka
> (2 rows)
>
> The problem seems to be in transformColumnDefinition, where the schema
> of the to-be-created sequence is determined from the relation name
> given. The default creation schema is used, if the user didn't specify
> the schame of the table explicitly, but since it's an ALTER TABLE, it
> really should use the schema of the existing table.

Correct.

> Patch against 8.2 attached, seems to apply to 8.1 and CVS head though I
> haven't tested them.. This is not my area of expertise, so I'm not 100%
> sure this is the right way to fix it.

I looked on it, but I think let parser to fill namespace information in
ctx->relation structure should be better then do it in this place. There
is also unfilled istemp flag.


        Zdenek

Zdenek Kotala wrote:

> I looked on it, but I think let parser to fill namespace information in
> ctx->relation structure should be better then do it in this place. There
> is also unfilled istemp flag.

Ignore this. It is good place.

However, I think add following function into namespace.c
should be nicer solution.

Oid  RelnameGetSchemaid(const char *relname);

See RelnameGetRelid.

You can use

snamespaceid = RelnameGetSchemaid(cxt->relation->relname);

instead of

snamespaceid = RangeVarGetCreationNamespace(cxt->relation);



    Zdenek

Heikki Linnakangas <heikki@enterprisedb.com> writes:
> 8.1 creates the sequence in wrong schema:

Yeah, 8.0 and 8.1 both do the wrong thing really, so it's hard to
argue that this ever worked.

> The problem seems to be in transformColumnDefinition, where the schema
> of the to-be-created sequence is determined from the relation name
> given. The default creation schema is used, if the user didn't specify
> the schame of the table explicitly, but since it's an ALTER TABLE, it
> really should use the schema of the existing table.

This is actually a bit nasty.  Your proposed patch doesn't really work,
because of the concern that is now commented at the head of
transformAlterTableStmt:

 * CAUTION: resist the temptation to do any work here that depends on the
 * current state of the table.  Actual execution of the command might not
 * occur till some future transaction.  Hence, we do only purely syntactic
 * transformations here, comparable to the processing of CREATE TABLE.

IOW, we don't actually *know* at parse analysis time which table will be
affected.

It's arguable that CREATE TABLE with a serial is broken too, because
conceivably search_path could change between parsing and execution
of the command, leading to the table being created in the new default
schema while the sequence still goes into the old one.

It looks to me like a "proper" fix requires postponing the formation of
the CREATE SEQUENCE command until execution time, when we can know with
some confidence what schema the table is in.  Yech.  That'll be pretty
invasive ... is it worth the trouble?

A possible alternative is to interpret CREATE/ALTER TABLE as nailing
down the target schema at parse analysis time, ie, after analysis the
query always looks as if you had written an explicit schema name rather
than leaving it up to search_path.  But this would be a behavioral
change that would likely bite somebody; and it would be inconsistent
with the behavior of other utility commands.

Maybe we should give up doing any CREATE/ALTER processing at all at
parse analysis time, and push it all to execution time.  I got rid of
parse-time processing of other utility statements during the plan
caching work a couple months ago, because of concerns very much like
this, but I hadn't bit the bullet for CREATE/ALTER TABLE because it was
such a huge chunk of code.  But maybe we'd better do it.

Comments?

            regards, tom lane

Tom Lane wrote:
> This is actually a bit nasty.  Your proposed patch doesn't really work,
> because of the concern that is now commented at the head of
> transformAlterTableStmt:
>
>  * CAUTION: resist the temptation to do any work here that depends on the
>  * current state of the table.  Actual execution of the command might not
>  * occur till some future transaction.  Hence, we do only purely syntactic
>  * transformations here, comparable to the processing of CREATE TABLE.
>
> IOW, we don't actually *know* at parse analysis time which table will be
> affected.

I don't understand that. Why would the execution be delayed to a future
transaction? You can't PREPARE an ALTER TABLE, right?

According to the comments in transformInhRelation, it has the same
problem...

> Maybe we should give up doing any CREATE/ALTER processing at all at
> parse analysis time, and push it all to execution time.  I got rid of
> parse-time processing of other utility statements during the plan
> caching work a couple months ago, because of concerns very much like
> this, but I hadn't bit the bullet for CREATE/ALTER TABLE because it was
> such a huge chunk of code.  But maybe we'd better do it.

We'll still need something smaller to back patch, I think. :(

--
   Heikki Linnakangas
   EnterpriseDB   http://www.enterprisedb.com

Heikki Linnakangas <heikki@enterprisedb.com> writes:
> Tom Lane wrote:
>> IOW, we don't actually *know* at parse analysis time which table will be
>> affected.

> I don't understand that. Why would the execution be delayed to a future
> transaction? You can't PREPARE an ALTER TABLE, right?

Yeah, you can.  Consider plpgsql, or protocol-level Bind.  The fact that
we don't expose these facilities as SQL doesn't mean they're not there.
A cached statement in plpgsql is actually the main case I'm worried
about...

>> Maybe we should give up doing any CREATE/ALTER processing at all at
>> parse analysis time, and push it all to execution time.

> We'll still need something smaller to back patch, I think. :(

At this point I don't think we'll try to fix this in the back branches.
It's never really worked, so I don't see 8.2's behavior as a regression,
and I don't see a small fix that doesn't create issues of its own.

            regards, tom lane