Re: [BUGS] BUG #7763: "CREATE TABLE ... (LIKE ... INCLUDING INDEXES ...)" does not work with indexes on composite types
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 +, no...@nix.hu wrote: > > > The following bug has been logged on the website: > > > > > > Bug reference: 7763 > > > Logged by: Norbert Buchmuller > > > Email address: no...@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 -- Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-bugs
Re: [BUGS] BUG #7763: "CREATE TABLE ... (LIKE ... INCLUDING INDEXES ...)" does not work with indexes on composite types
Andres Freund 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 -- Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-bugs
Re: [BUGS] BUG #7763: "CREATE TABLE ... (LIKE ... INCLUDING INDEXES ...)" does not work with indexes on composite types
Hi,
On 2012-12-22 16:11:47 -0500, Tom Lane wrote:
> Andres Freund 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
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
