Обсуждение: BUG #7763: "CREATE TABLE ... (LIKE ... INCLUDING INDEXES ...)" does not work with indexes on composite types
The following bug has been logged on the website: Bug reference: 7763 Logged by: Norbert Buchmuller Email address: norbi@nix.hu PostgreSQL version: 9.2.2 Operating system: Linux 2.6.32, i386, Debian GNU/Linux 6.0.5 Description: = There's a table that has a B-Tree index on a composite type expression. When attempting to create another table just like the first table and with the indexes also "copied" using the "CREATE TABLE ... (LIKE ... INCLUDING INDEXES ...)" statement, it throws an error (see below) and the table is not created. I believe it's a bug, from the documentation i assumed that it should create the table with a similar index, no matter that the index is on a composite type expression. postgres@vger:~$ cat create_table_like_including_indexes-and-index_on_composite_type.sql = \set VERBOSITY verbose = \set ECHO all SELECT version(); CREATE TYPE type1 AS (x int, y int); CREATE TABLE table1 (a int, b int); CREATE INDEX index1 ON table1 ( ( (a, b)::type1 ) ); CREATE TABLE table2 ( LIKE table1 INCLUDING INDEXES ); \d table2 postgres@vger:~$ dropdb test1; createdb test1 && psql --no-align --tuples -d test1 -f create_table_like_including_indexes-and-index_on_composite_type.sql SELECT version(); PostgreSQL 9.2.2 on i686-pc-linux-gnu, compiled by gcc-4.4.real (Debian 4.4.5-8) 4.4.5, 32-bit CREATE TYPE type1 AS (x int, y int); CREATE TYPE CREATE TABLE table1 (a int, b int); CREATE TABLE CREATE INDEX index1 ON table1 ( ( (a, b)::type1 ) ); CREATE INDEX CREATE TABLE table2 ( LIKE table1 INCLUDING INDEXES ); psql:create_table_like_including_indexes-and-index_on_composite_type.sql:7: ERROR: 42P16: column "" has pseudo-type record LOCATION: CheckAttributeType, heap.c:496 \d table2 Did not find any relation named "table2".
On 2012-12-20 12:40:44 +0000, norbi@nix.hu wrote: > The following bug has been logged on the website: > > Bug reference: 7763 > Logged by: Norbert Buchmuller > Email address: norbi@nix.hu > PostgreSQL version: 9.2.2 > Operating system: Linux 2.6.32, i386, Debian GNU/Linux 6.0.5 > Description: > > There's a table that has a B-Tree index on a composite type expression. When > attempting to create another table just like the first table and with the > indexes also "copied" using the "CREATE TABLE ... (LIKE ... INCLUDING > INDEXES ...)" statement, it throws an error (see below) and the table is not > created. > > I believe it's a bug, from the documentation i assumed that it should create > the table with a similar index, no matter that the index is on a composite > type expression. > > postgres@vger:~$ cat > create_table_like_including_indexes-and-index_on_composite_type.sql > \set VERBOSITY verbose > \set ECHO all > SELECT version(); > CREATE TYPE type1 AS (x int, y int); > CREATE TABLE table1 (a int, b int); > CREATE INDEX index1 ON table1 ( ( (a, b)::type1 ) ); > CREATE TABLE table2 ( LIKE table1 INCLUDING INDEXES ); > \d table2 > postgres@vger:~$ dropdb test1; createdb test1 && psql --no-align --tuples -d > test1 -f create_table_like_including_indexes-and-index_on_composite_type.sql > > SELECT version(); > PostgreSQL 9.2.2 on i686-pc-linux-gnu, compiled by gcc-4.4.real (Debian > 4.4.5-8) 4.4.5, 32-bit > CREATE TYPE type1 AS (x int, y int); > CREATE TYPE > CREATE TABLE table1 (a int, b int); > CREATE TABLE > CREATE INDEX index1 ON table1 ( ( (a, b)::type1 ) ); > CREATE INDEX > CREATE TABLE table2 ( LIKE table1 INCLUDING INDEXES ); > psql:create_table_like_including_indexes-and-index_on_composite_type.sql:7: > ERROR: 42P16: column "" has pseudo-type record > LOCATION: CheckAttributeType, heap.c:496 > \d table2 > Did not find any relation named "table2". Concretely that seems to be transformRowExpr's fault. It overwrites row_typeid even though its marked as COERCE_EXPLICIT_CAST. Now the original problem seems to be that we are transforming an already transformed expression. generateClonedIndexStmt gets the expression from the old index via nodeToString, remaps some attnos, but thats about it. ISTM IndexElem grow a cooked_expr member. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On 2012-12-20 21:17:04 +0100, Andres Freund wrote: > On 2012-12-20 12:40:44 +0000, norbi@nix.hu wrote: > > The following bug has been logged on the website: > > > > Bug reference: 7763 > > Logged by: Norbert Buchmuller > > Email address: norbi@nix.hu > > PostgreSQL version: 9.2.2 > > Operating system: Linux 2.6.32, i386, Debian GNU/Linux 6.0.5 > > Description: > > > > There's a table that has a B-Tree index on a composite type expression. When > > attempting to create another table just like the first table and with the > > indexes also "copied" using the "CREATE TABLE ... (LIKE ... INCLUDING > > INDEXES ...)" statement, it throws an error (see below) and the table is not > > created. > > > > I believe it's a bug, from the documentation i assumed that it should create > > the table with a similar index, no matter that the index is on a composite > > type expression. > > > > postgres@vger:~$ cat > > create_table_like_including_indexes-and-index_on_composite_type.sql > > \set VERBOSITY verbose > > \set ECHO all > > SELECT version(); > > CREATE TYPE type1 AS (x int, y int); > > CREATE TABLE table1 (a int, b int); > > CREATE INDEX index1 ON table1 ( ( (a, b)::type1 ) ); > > CREATE TABLE table2 ( LIKE table1 INCLUDING INDEXES ); > > \d table2 > > postgres@vger:~$ dropdb test1; createdb test1 && psql --no-align --tuples -d > > test1 -f create_table_like_including_indexes-and-index_on_composite_type.sql > > > > SELECT version(); > > PostgreSQL 9.2.2 on i686-pc-linux-gnu, compiled by gcc-4.4.real (Debian > > 4.4.5-8) 4.4.5, 32-bit > > CREATE TYPE type1 AS (x int, y int); > > CREATE TYPE > > CREATE TABLE table1 (a int, b int); > > CREATE TABLE > > CREATE INDEX index1 ON table1 ( ( (a, b)::type1 ) ); > > CREATE INDEX > > CREATE TABLE table2 ( LIKE table1 INCLUDING INDEXES ); > > psql:create_table_like_including_indexes-and-index_on_composite_type.sql:7: > > ERROR: 42P16: column "" has pseudo-type record > > LOCATION: CheckAttributeType, heap.c:496 > > \d table2 > > Did not find any relation named "table2". > > Concretely that seems to be transformRowExpr's fault. It overwrites > row_typeid even though its marked as COERCE_EXPLICIT_CAST. > > Now the original problem seems to be that we are transforming an already > transformed expression. generateClonedIndexStmt gets the expression from > the old index via nodeToString, remaps some attnos, but thats about > it. > ISTM IndexElem grow a cooked_expr member. +should Ok, here are two patches: * Add a cooked_expr member to IndexElem and use it for transformed expressions, including filling it directly in generateClonedIndexStmt. * Follow the pattern set by other routines in parse_expr.c and don't transformRowExpr the same expression twice. While the first one fixes the above bug - and I think its the right approach not to analyze the expression twice, the second one seems like a good idea anyway because as transformExpr says: * 1. At least one construct (BETWEEN/AND) puts the same nodes * into two branches of the parse tree; hence, some nodes * are transformed twice. * 2. Another way it can happen is that coercion of an operator or * function argument to the required type (via coerce_type()) * can apply transformExpr to an already-transformed subexpression. * An example here is "SELECT count(*) + 1.0 FROM table". There unfortunately is not sufficient padding in IndexElem to do that without changing its size. Not sure whether we consider that to be a big problem for the back branches, its nothing user code should do, but ... Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Вложения
On 2012-12-20 22:50:54 +0100, Andres Freund wrote: > On 2012-12-20 21:17:04 +0100, Andres Freund wrote: > > On 2012-12-20 12:40:44 +0000, norbi@nix.hu wrote: > > > The following bug has been logged on the website: > > > > > > Bug reference: 7763 > > > Logged by: Norbert Buchmuller > > > Email address: norbi@nix.hu > > > PostgreSQL version: 9.2.2 > > > Operating system: Linux 2.6.32, i386, Debian GNU/Linux 6.0.5 > > > Description: > > > > > > There's a table that has a B-Tree index on a composite type expression. When > > > attempting to create another table just like the first table and with the > > > indexes also "copied" using the "CREATE TABLE ... (LIKE ... INCLUDING > > > INDEXES ...)" statement, it throws an error (see below) and the table is not > > > created. > > > > > > I believe it's a bug, from the documentation i assumed that it should create > > > the table with a similar index, no matter that the index is on a composite > > > type expression. > > > > > > postgres@vger:~$ cat > > > create_table_like_including_indexes-and-index_on_composite_type.sql > > > \set VERBOSITY verbose > > > \set ECHO all > > > SELECT version(); > > > CREATE TYPE type1 AS (x int, y int); > > > CREATE TABLE table1 (a int, b int); > > > CREATE INDEX index1 ON table1 ( ( (a, b)::type1 ) ); > > > CREATE TABLE table2 ( LIKE table1 INCLUDING INDEXES ); > > > \d table2 > > > postgres@vger:~$ dropdb test1; createdb test1 && psql --no-align --tuples -d > > > test1 -f create_table_like_including_indexes-and-index_on_composite_type.sql > > > > > > SELECT version(); > > > PostgreSQL 9.2.2 on i686-pc-linux-gnu, compiled by gcc-4.4.real (Debian > > > 4.4.5-8) 4.4.5, 32-bit > > > CREATE TYPE type1 AS (x int, y int); > > > CREATE TYPE > > > CREATE TABLE table1 (a int, b int); > > > CREATE TABLE > > > CREATE INDEX index1 ON table1 ( ( (a, b)::type1 ) ); > > > CREATE INDEX > > > CREATE TABLE table2 ( LIKE table1 INCLUDING INDEXES ); > > > psql:create_table_like_including_indexes-and-index_on_composite_type.sql:7: > > > ERROR: 42P16: column "" has pseudo-type record > > > LOCATION: CheckAttributeType, heap.c:496 > > > \d table2 > > > Did not find any relation named "table2". > > > > Concretely that seems to be transformRowExpr's fault. It overwrites > > row_typeid even though its marked as COERCE_EXPLICIT_CAST. > > > > Now the original problem seems to be that we are transforming an already > > transformed expression. generateClonedIndexStmt gets the expression from > > the old index via nodeToString, remaps some attnos, but thats about > > it. > > ISTM IndexElem grow a cooked_expr member. > > +should > > Ok, here are two patches: > * Add a cooked_expr member to IndexElem and use it for transformed > expressions, including filling it directly in generateClonedIndexStmt. > > * Follow the pattern set by other routines in parse_expr.c and don't > transformRowExpr the same expression twice. > > While the first one fixes the above bug - and I think its the right > approach not to analyze the expression twice, the second one seems like > a good idea anyway because as transformExpr says: > * 1. At least one construct (BETWEEN/AND) puts the same nodes > * into two branches of the parse tree; hence, some nodes > * are transformed twice. > * 2. Another way it can happen is that coercion of an operator or > * function argument to the required type (via coerce_type()) > * can apply transformExpr to an already-transformed subexpression. > * An example here is "SELECT count(*) + 1.0 FROM table". > > There unfortunately is not sufficient padding in IndexElem to do that > without changing its size. Not sure whether we consider that to be a big > problem for the back branches, its nothing user code should do, but > ... So nobody has an idea that would avoid changing the sizeof(IndexElem)? We could just apply patch 2 to fix the issue at hand, but I am pretty sure transforming the whole expression twice can create other problems than just this. IndexStmt has some padding available at the end, we could add a bool "precooked" there, but that seems to be rather ugly. Greetings, Andres -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Andres Freund <andres@2ndquadrant.com> writes:
> On 2012-12-20 22:50:54 +0100, Andres Freund wrote:
>> Ok, here are two patches:
>> * Add a cooked_expr member to IndexElem and use it for transformed
>> expressions, including filling it directly in generateClonedIndexStmt.
>>
>> * Follow the pattern set by other routines in parse_expr.c and don't
>> transformRowExpr the same expression twice.
>>
>> While the first one fixes the above bug - and I think its the right
>> approach not to analyze the expression twice, the second one seems like
>> a good idea anyway because as transformExpr says:
>> * 1. At least one construct (BETWEEN/AND) puts the same nodes
>> * into two branches of the parse tree; hence, some nodes
>> * are transformed twice.
>> * 2. Another way it can happen is that coercion of an operator or
>> * function argument to the required type (via coerce_type())
>> * can apply transformExpr to an already-transformed subexpression.
>> * An example here is "SELECT count(*) + 1.0 FROM table".
>>
>> There unfortunately is not sufficient padding in IndexElem to do that
>> without changing its size. Not sure whether we consider that to be a big
>> problem for the back branches, its nothing user code should do, but
>> ...
> So nobody has an idea that would avoid changing the sizeof(IndexElem)?
Yeah: forget the first patch and just do the second. There are already
sufficient reasons why transformExpr has to be idempotent; this is just
another one. I don't really see a need to kluge up IndexElem for this.
We might at some point try to clean all this up. But in the meantime
I see no good reason to make LIKE INCLUDING INDEXES adhere to a higher
standard than the rest of the code does, and even less reason to
back-patch such a change.
BTW, it sure looks to me like transformXmlExpr will get an Assert
failure on an already-transformed expression ...
regards, tom lane
Hi,
On 2012-12-22 16:11:47 -0500, Tom Lane wrote:
> Andres Freund <andres@2ndquadrant.com> writes:
> > On 2012-12-20 22:50:54 +0100, Andres Freund wrote:
> >> Ok, here are two patches:
> >> * Add a cooked_expr member to IndexElem and use it for transformed
> >> expressions, including filling it directly in generateClonedIndexStmt.
> >>
> >> * Follow the pattern set by other routines in parse_expr.c and don't
> >> transformRowExpr the same expression twice.
> >>
> >> While the first one fixes the above bug - and I think its the right
> >> approach not to analyze the expression twice, the second one seems like
> >> a good idea anyway because as transformExpr says:
> >> * 1. At least one construct (BETWEEN/AND) puts the same nodes
> >> * into two branches of the parse tree; hence, some nodes
> >> * are transformed twice.
> >> * 2. Another way it can happen is that coercion of an operator or
> >> * function argument to the required type (via coerce_type())
> >> * can apply transformExpr to an already-transformed subexpression.
> >> * An example here is "SELECT count(*) + 1.0 FROM table".
> >>
> >> There unfortunately is not sufficient padding in IndexElem to do that
> >> without changing its size. Not sure whether we consider that to be a big
> >> problem for the back branches, its nothing user code should do, but
> >> ...
>
> > So nobody has an idea that would avoid changing the sizeof(IndexElem)?
>
> Yeah: forget the first patch and just do the second. There are already
> sufficient reasons why transformExpr has to be idempotent; this is just
> another one. I don't really see a need to kluge up IndexElem for this.
I understand that position, its just, neither a constraint' nor a
default argument's expr currently have multiple evaluation
transformation dangers as they currently have an explicit representation
(different ones actually) of raw/cooked expressions. ISTM that the
guarantee of idempotent transformExpr is minimally tested outside of the
usually trivial cases that transformExpr's comment documents.
So kludging up IndexElem or IndexStmt seems like the safer choice.
Making sure transformExpr is actually idempotent seems hard after a
very quick look through parse_expr.c and friends.
But I am happy with providing an extended patch that fixes the OP's and
the case you noticed. I will also make a pass and look for other obvious
things. Those should be fixed independently of 1).
I wonder if we shouldn't at least do something roughly akin to
#ifdef USE_ASSERT_CHECKING
result = transformExprRecurse(pstate, expr);
if (assert_enabled)
{
Node *copy = copyObject(result);
copy = transformExprRecurse(copy, result);
Assert(equal(result, copy));
}
#endif
in HEAD and check whether it survives make check.
> We might at some point try to clean all this up. But in the meantime
> I see no good reason to make LIKE INCLUDING INDEXES adhere to a higher
> standard than the rest of the code does, and even less reason to
> back-patch such a change.
ISTM most things do adhere to that higher standard, thats why I had
thought patch 1 might be a good idea...
> BTW, it sure looks to me like transformXmlExpr will get an Assert
> failure on an already-transformed expression ...
Yep. I am pretty sure there are others :(
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Andres Freund <andres@2ndquadrant.com> writes:
> On 2012-12-22 16:11:47 -0500, Tom Lane wrote:
>> BTW, it sure looks to me like transformXmlExpr will get an Assert
>> failure on an already-transformed expression ...
> Yep. I am pretty sure there are others :(
Eyeball inspection doesn't find any: either the output node type is
different, or the function has a guard against repeat analysis, or
it actually is idempotent anyway.
I plan to commit a fix that just adds guards to transformRowExpr and
transformXmlExpr. If you feel like expending more work in this area,
I'd suggest looking into what it would take to remove the requirement
of idempotence. We know at least three places that would need changes
--- what others are there?
regards, tom lane