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