Thread
Commits
Same data as JSON:
GET /api/v1/messages/:b64id/commits
the thread's linked commits as JSON, with link sources.
API reference →
-
In ExecInitModifyTable, don't scribble on the source plan.
- d376ab570ef9 18.0 landed
-
Rely on executor utils to build targetlist for DML RETURNING.
- 4717fdb14cf0 11.0 cited
-
Violation of principle that plan trees are read-only
Tom Lane <tgl@sss.pgh.pa.us> — 2025-05-18T23:31:33Z
While chasing down Valgrind leakage reports, I was disturbed to realize that some of them arise from a case where the executor scribbles on the plan tree it's given, which it is absolutely not supposed to do: /* * Initialize result tuple slot and assign its rowtype using the first * RETURNING list. We assume the rest will look the same. */ mtstate->ps.plan->targetlist = (List *) linitial(returningLists); A bit of git archaeology fingers Andres' commit 4717fdb14, which we can't easily revert since he later got rid of ExecAssignResultType altogether. But I think we need to do something about it --- it's purest luck that this doesn't cause serious problems in some cases. regards, tom lane -
Re: Violation of principle that plan trees are read-only
Robert Haas <robertmhaas@gmail.com> — 2025-05-19T12:35:05Z
On Sun, May 18, 2025 at 7:31 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > While chasing down Valgrind leakage reports, I was disturbed > to realize that some of them arise from a case where the > executor scribbles on the plan tree it's given, which it is > absolutely not supposed to do: > > /* > * Initialize result tuple slot and assign its rowtype using the first > * RETURNING list. We assume the rest will look the same. > */ > mtstate->ps.plan->targetlist = (List *) linitial(returningLists); > > A bit of git archaeology fingers Andres' commit 4717fdb14, which we > can't easily revert since he later got rid of ExecAssignResultType > altogether. But I think we need to do something about it --- it's > purest luck that this doesn't cause serious problems in some cases. Is there some way that we can detect violations of this rule automatically? I recall that we were recently discussing with Richard Guo a proposed patch that would have had a similar problem, so it's evidently not that hard for a committer to either fail to understand what the rule is or fail to realize that they are violating it. -- Robert Haas EDB: http://www.enterprisedb.com
-
Re: Violation of principle that plan trees are read-only
Isaac Morland <isaac.morland@gmail.com> — 2025-05-19T12:41:06Z
On Mon, 19 May 2025 at 08:35, Robert Haas <robertmhaas@gmail.com> wrote: > On Sun, May 18, 2025 at 7:31 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > While chasing down Valgrind leakage reports, I was disturbed > > to realize that some of them arise from a case where the > > executor scribbles on the plan tree it's given, which it is > > absolutely not supposed to do: > > > > /* > > * Initialize result tuple slot and assign its rowtype using the > first > > * RETURNING list. We assume the rest will look the same. > > */ > > mtstate->ps.plan->targetlist = (List *) linitial(returningLists); > > > > A bit of git archaeology fingers Andres' commit 4717fdb14, which we > > can't easily revert since he later got rid of ExecAssignResultType > > altogether. But I think we need to do something about it --- it's > > purest luck that this doesn't cause serious problems in some cases. > > Is there some way that we can detect violations of this rule > automatically? I recall that we were recently discussing with Richard > Guo a proposed patch that would have had a similar problem, so it's > evidently not that hard for a committer to either fail to understand > what the rule is or fail to realize that they are violating it. I assume this question has an obvious negative answer, but why can't we attach const declarations to the various structures that make up the plan tree (at all levels, all the way down)? I know const doesn't actually prevent a value from changing, but at least the compiler would complain if code accidentally tried.
-
Re: Violation of principle that plan trees are read-only
Tom Lane <tgl@sss.pgh.pa.us> — 2025-05-19T14:35:26Z
Robert Haas <robertmhaas@gmail.com> writes: > On Sun, May 18, 2025 at 7:31 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: >> While chasing down Valgrind leakage reports, I was disturbed >> to realize that some of them arise from a case where the >> executor scribbles on the plan tree it's given, which it is >> absolutely not supposed to do: > Is there some way that we can detect violations of this rule > automatically? I recall that we were recently discussing with Richard > Guo a proposed patch that would have had a similar problem, so it's > evidently not that hard for a committer to either fail to understand > what the rule is or fail to realize that they are violating it. I proposed a possible way to test for this at [1]. I was intending to get around to that sooner or later, but the urgency of the matter just went up in my eyes... regards, tom lane [1] https://www.postgresql.org/message-id/flat/2531459.1743871597%40sss.pgh.pa.us
-
Re: Violation of principle that plan trees are read-only
Robert Haas <robertmhaas@gmail.com> — 2025-05-19T14:39:51Z
On Mon, May 19, 2025 at 10:35 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > I proposed a possible way to test for this at [1]. I was intending to > get around to that sooner or later, but the urgency of the matter just > went up in my eyes... Ah, right, I actually read that thread but had forgotten about it. I don't know if that idea will work out but it certainly seems like a good thing to try. -- Robert Haas EDB: http://www.enterprisedb.com
-
Re: Violation of principle that plan trees are read-only
Tom Lane <tgl@sss.pgh.pa.us> — 2025-05-19T14:45:47Z
Isaac Morland <isaac.morland@gmail.com> writes: > I assume this question has an obvious negative answer, but why can't we > attach const declarations to the various structures that make up the plan > tree (at all levels, all the way down)? I know const doesn't actually > prevent a value from changing, but at least the compiler would complain if > code accidentally tried. The big problem is that a "const" attached to a top-level pointer doesn't inherently propagate down to sub-nodes. So if I had, say, "const Query *stmt", the compiler would complain about stmt->jointree = foo; but not about stmt->jointree->quals = foo; I guess we could imagine developing an entirely parallel set of struct declarations with "const" on all pointer fields, like typedef struct ConstQuery { ... const ConstFromExpr *jointree; ... } ConstQuery; but even with automated maintenance of the ConstFoo doppelganger typedefs, it seems like that'd be a notational nightmare. For one thing, I'm not sure how to teach the compiler that casting "Query *" to "ConstQuery *" is okay but vice versa isn't. Does C++ have a better story in this area? I haven't touched it in so long that I don't remember. regards, tom lane -
Re: Violation of principle that plan trees are read-only
Jose Luis Tallon <jltallon@adv-solutions.net> — 2025-05-19T15:49:59Z
On 19/5/25 16:45, Tom Lane wrote: > [snip] > For one thing, I'm not sure how to teach the compiler that casting > "Query *" to "ConstQuery *" is okay but vice versa isn't. > > Does C++ have a better story in this area? Hi, A C++ compiler *does* enforce "const correctness" (for examples see, for example, Meyer's fantastic "Efficient C++" series). It seems that Postgres is approaching the class of complexity/maturity that (modern) C++ was designed to solve. It should be unmatched in that area. I believe that, with recent platform deprecations, the vast majority of systems where one can compile a modern Postgres should already have a decent C++ compiler available (g++ & clang probably cover >95%) As opposed to say, rewriting in Rust ---where the compilar also does a very substantial checking of semantics at compile time---, introducing some C++ in Postgres could be as simple as throwing some "#ifdef __cplusplus__ extern "C" { #endif" in the headers and use the required subset in certain modules *only*. (this is how, for instance, Bacula did it ~two decades ago) The planner and parts of the bufmgr / arenas (palloc et al.) seem to me as the most obvious candidates to try. I'm not sure whether it'd introduce any unintended regressions, though. > I haven't touched it > in so long that I don't remember. C++ 17 would be the standard to tackle, IMHO. HTH, J.L. -- Parkinson's Law: Work expands to fill the time alloted to it. -
Re: Violation of principle that plan trees are read-only
Andres Freund <andres@anarazel.de> — 2025-05-20T14:59:22Z
Hi, On 2025-05-18 19:31:33 -0400, Tom Lane wrote: > While chasing down Valgrind leakage reports, I was disturbed > to realize that some of them arise from a case where the > executor scribbles on the plan tree it's given, which it is > absolutely not supposed to do: > > /* > * Initialize result tuple slot and assign its rowtype using the first > * RETURNING list. We assume the rest will look the same. > */ > mtstate->ps.plan->targetlist = (List *) linitial(returningLists); > > A bit of git archaeology fingers Andres' commit 4717fdb14, which we > can't easily revert since he later got rid of ExecAssignResultType > altogether. But I think we need to do something about it --- it's > purest luck that this doesn't cause serious problems in some cases. I have no idea what I was smoking at that time, this clearly is wrong. I think the reason it doesn't cause problems is that we're just using the first child's targetlist every time and we're just assigning the same value back every time. What's even more absurd is: Why do we even need to assign the result type at all? Before & after 4717fdb14. The planner ought to have already figured this out, no? It seems that if not, we'd have a problem anyway, who says the "calling" nodes (say a wCTE) can cope with whatever output we come up with? Except of course, there is exactly one case in our tests where the tupledescs aren't equal :( I've not dug fully into this, but I thought I should send this email to confirm that I'm looking into the issue. Greetings, Andres Freund
-
Re: Violation of principle that plan trees are read-only
Andres Freund <andres@anarazel.de> — 2025-05-20T15:54:31Z
Hi, On 2025-05-20 10:59:22 -0400, Andres Freund wrote: > On 2025-05-18 19:31:33 -0400, Tom Lane wrote: > > While chasing down Valgrind leakage reports, I was disturbed > > to realize that some of them arise from a case where the > > executor scribbles on the plan tree it's given, which it is > > absolutely not supposed to do: > > > > /* > > * Initialize result tuple slot and assign its rowtype using the first > > * RETURNING list. We assume the rest will look the same. > > */ > > mtstate->ps.plan->targetlist = (List *) linitial(returningLists); > > > > A bit of git archaeology fingers Andres' commit 4717fdb14, which we > > can't easily revert since he later got rid of ExecAssignResultType > > altogether. But I think we need to do something about it --- it's > > purest luck that this doesn't cause serious problems in some cases. > > I have no idea what I was smoking at that time, this clearly is wrong. > > I think the reason it doesn't cause problems is that we're just using the > first child's targetlist every time and we're just assigning the same value > back every time. > > > What's even more absurd is: Why do we even need to assign the result type at > all? Before & after 4717fdb14. The planner ought to have already figured this > out, no? > > It seems that if not, we'd have a problem anyway, who says the "calling" nodes > (say a wCTE) can cope with whatever output we come up with? > > Except of course, there is exactly one case in our tests where the tupledescs > aren't equal :( That's a harmless difference, as it turns out. The varnos / varattnos differ, but that's fine, because we don't actually build the projection with that tlist, we just use to allocate the slot, and that doesn't care about the tlist. The building of the projection uses the specific child's returning list. Afaict the mtstate->ps.plan->targetlist assignment, and the ExecTypeFromTL(), ExecAssignResultTypeFromTL() before that, are completely superfluous. Am I missing something? Wonder if the targetlist assignment is superfluous made me wonder if we would detect mismatches - and afaict we largely wouldn't. There's basically no verification in ExecBuildProjectionInfo(). And indeed, adding something very basic shows: --- /home/andres/src/postgresql/src/test/regress/expected/merge.out 2025-04-06 22:54:14.078394968 -0400 +++ /srv/dev/build/postgres/m-dev-assert/testrun/regress/regress/results/merge.out 2025-05-20 11:51:51.549525728 -0400 @@ -2653,20 +2653,95 @@ MERGE into measurement m USING new_measurement nm ON (m.city_id = nm.city_id and m.logdate=nm.logdate) WHEN MATCHED AND nm.peaktemp IS NULL THEN DELETE WHEN MATCHED THEN UPDATE SET peaktemp = greatest(m.peaktemp, nm.peaktemp), unitsales = m.unitsales + coalesce(nm.unitsales, 0) WHEN NOT MATCHED THEN INSERT (city_id, logdate, peaktemp, unitsales) VALUES (city_id, logdate, peaktemp, unitsales); +WARNING: type mismatch: resno 1: slot type 0 vs expr type 23 +WARNING: TargetEntry: +DETAIL: {TARGETENTRY + :expr + {VAR + :varno -1 + :varattno 3 + :vartype 23 + :vartypmod -1 + :varcollid 0 + :varnullingrels (b) + :varlevelsup 0 + :varreturningtype 0 + :varnosyn 2 + :varattnosyn 1 + :location 384 + } + :resno 1 + :resname city_id + :ressortgroupref 0 + :resorigtbl 0 + :resorigcol 0 + :resjunk false + } + +WARNING: type mismatch: resno 2: slot type 23 vs expr type 1082 +WARNING: TargetEntry: +DETAIL: {TARGETENTRY + :expr + {VAR + :varno -1 + :varattno 4 + :vartype 1082 + :vartypmod -1 + :varcollid 0 + :varnullingrels (b) + :varlevelsup 0 + :varreturningtype 0 + :varnosyn 2 + :varattnosyn 2 + :location 393 + } + :resno 2 + :resname logdate + :ressortgroupref 0 + :resorigtbl 0 + :resorigcol 0 + :resjunk false + } + +WARNING: type mismatch: resno 3: slot type 1082 vs expr type 23 +WARNING: TargetEntry: +DETAIL: {TARGETENTRY + :expr + {VAR + :varno -1 + :varattno 1 + :vartype 23 + :vartypmod -1 + :varcollid 0 + :varnullingrels (b) + :varlevelsup 0 + :varreturningtype 0 + :varnosyn 2 + :varattnosyn 3 + :location 402 + } + :resno 3 ... Greetings, Andres Freund -
Re: Violation of principle that plan trees are read-only
Tom Lane <tgl@sss.pgh.pa.us> — 2025-05-20T16:36:02Z
Andres Freund <andres@anarazel.de> writes: > Afaict the mtstate->ps.plan->targetlist assignment, and the ExecTypeFromTL(), > ExecAssignResultTypeFromTL() before that, are completely superfluous. Am I > missing something? I think you are right. The two tlists are completely identical in most cases, because of this bit in setrefs.c: /* * Set up the visible plan targetlist as being the same as * the first RETURNING list. This is for the use of * EXPLAIN; the executor won't pay any attention to the * targetlist. We postpone this step until here so that * we don't have to do set_returning_clause_references() * twice on identical targetlists. */ splan->plan.targetlist = copyObject(linitial(newRL)); I added a quick check + if (!equal(mtstate->ps.plan->targetlist, + linitial(returningLists))) + elog(WARNING, "not matched targetlist"); to verify that. There's one regression case in which it complains, and that's one where we pruned the first result relation so that linitial(returningLists) is now the second result rel's RETURNING list. But as you say, that should still produce the same tupdesc. I think we can just delete this assignment (and fix these comments). > Wonder if the targetlist assignment is superfluous made me wonder if we would > detect mismatches - and afaict we largely wouldn't. There's basically no > verification in ExecBuildProjectionInfo(). And indeed, adding something very > basic shows: Hmm, seems like an independent issue. regards, tom lane -
Re: Violation of principle that plan trees are read-only
Tom Lane <tgl@sss.pgh.pa.us> — 2025-05-20T17:04:46Z
I wrote: > I think we can just delete this assignment (and fix these comments). As attached. I'm tempted to back-patch this: the plan tree damage seems harmless at present, but maybe it'd become less harmless with future fixes. regards, tom lane
-
Re: Violation of principle that plan trees are read-only
Andres Freund <andres@anarazel.de> — 2025-05-20T18:23:25Z
Hi, On 2025-05-20 13:04:46 -0400, Tom Lane wrote: > I wrote: > > I think we can just delete this assignment (and fix these comments). > > As attached. Largely makes sense, the only thing I see is that the !returningLists branch does: /* * We still must construct a dummy result tuple type, because InitPlan * expects one (maybe should change that?). */ mtstate->ps.plan->targetlist = NIL; which we presumably shouldn't do anymore either. It never changes anything afaict, but still. > I'm tempted to back-patch this: the plan tree damage seems harmless at > present, but maybe it'd become less harmless with future fixes. I am not sure, I can see arguments either way. <ponder> There are *some* cases where this changes the explain output, but but the new output is more correct, I think: --- /tmp/a.out 2025-05-20 14:19:04.947945026 -0400 +++ /tmp/b.out 2025-05-20 14:19:12.878975702 -0400 @@ -18,7 +18,7 @@ QUERY PLAN ----------------------------------------------------------------------------- Update on public.part_abc - Output: part_abc_1.a, part_abc_1.b, part_abc_1.c + Output: a, b, c Update on public.part_abc_2 part_abc_1 -> Append Subplans Removed: 1 I suspect this is an argument for backpatching, not against - seems that deparsing could end up creating bogus output in cases where it could matter? Not sure if such cases are reachable via views (and thus pg_dump) or postgres_fdw, but it seems possible. Greetings, Andres Freund -
Re: Violation of principle that plan trees are read-only
Tom Lane <tgl@sss.pgh.pa.us> — 2025-05-20T20:18:57Z
Andres Freund <andres@anarazel.de> writes: > Largely makes sense, the only thing I see is that the !returningLists branch > does: > /* > * We still must construct a dummy result tuple type, because InitPlan > * expects one (maybe should change that?). > */ > mtstate->ps.plan->targetlist = NIL; > which we presumably shouldn't do anymore either. It never changes anything > afaict, but still. D'oh ... I had seen that branch before, but missed fixing it. Yeah, the targetlist will be NIL already, but it's still wrong. >> I'm tempted to back-patch this: the plan tree damage seems harmless at >> present, but maybe it'd become less harmless with future fixes. > There are *some* cases where this changes the explain output, but but the new > output is more correct, I think: > ... > I suspect this is an argument for backpatching, not against - seems that > deparsing could end up creating bogus output in cases where it could matter? > Not sure if such cases are reachable via views (and thus pg_dump) or > postgres_fdw, but it seems possible. I don't believe that we guarantee EXPLAIN output to be 100% valid SQL, so I doubt there's a correctness argument here; certainly it'd not affect pg_dump. I'm curious though: what was the test case you were looking at? regards, tom lane
-
Re: Violation of principle that plan trees are read-only
Andres Freund <andres@anarazel.de> — 2025-05-20T20:40:34Z
Hi, On 2025-05-20 16:18:57 -0400, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > >> I'm tempted to back-patch this: the plan tree damage seems harmless at > >> present, but maybe it'd become less harmless with future fixes. > > > There are *some* cases where this changes the explain output, but but the new > > output is more correct, I think: > > ... > > I suspect this is an argument for backpatching, not against - seems that > > deparsing could end up creating bogus output in cases where it could matter? > > Not sure if such cases are reachable via views (and thus pg_dump) or > > postgres_fdw, but it seems possible. > > I don't believe that we guarantee EXPLAIN output to be 100% valid SQL, > so I doubt there's a correctness argument here; certainly it'd not > affect pg_dump. I wasn't thinking of EXPLAIN itself, but was wondering whether it's possible to create a view, rule or such that is affected by the output change. Would be a weird case, if it existed. > I'm curious though: what was the test case you were looking at? It's a modified query from our regression tests, I had added some debugging to find cases where the targetlists differed. I attached the extracted, somewhat modified, sql script. Greetings, Andres Freund
-
Re: Violation of principle that plan trees are read-only
Nico Williams <nico@cryptonector.com> — 2025-05-20T20:43:55Z
On Mon, May 19, 2025 at 08:41:06AM -0400, Isaac Morland wrote: > I assume this question has an obvious negative answer, but why can't we > attach const declarations to the various structures that make up the plan > tree (at all levels, all the way down)? I know const doesn't actually > prevent a value from changing, but at least the compiler would complain if > code accidentally tried. What you want is for C to have a type attribute that denotes immutability all the way down. `const` doesn't do that. One thing that could be done is to write a utility that creates const-all-the-way-down clones of given types, but such a tool can't be written as C pre-processor macros -- it would have to be a tool that parses the actual type definitions, or uses DWARF or similar to from the compilation of a file (I've done this latter before, but this weds you to compilers that output DWARF, which MSVC doesn't, for example). Really, the C committee ought to add this at some point, darn it. It would be the opposite of Rust's &mut. Nico --
-
Re: Violation of principle that plan trees are read-only
Tom Lane <tgl@sss.pgh.pa.us> — 2025-05-20T21:01:11Z
Andres Freund <andres@anarazel.de> writes: > On 2025-05-20 16:18:57 -0400, Tom Lane wrote: >> I'm curious though: what was the test case you were looking at? > It's a modified query from our regression tests, I had added some debugging to > find cases where the targetlists differed. I attached the extracted, somewhat > modified, sql script. [ confused... ] I get the same output from that script with or without the patch. regards, tom lane
-
Re: Violation of principle that plan trees are read-only
Tom Lane <tgl@sss.pgh.pa.us> — 2025-05-20T21:04:56Z
I wrote: > [ confused... ] I get the same output from that script with or > without the patch. Oh, scratch that: I'd gotten confused about which branch I was working in. It does change the output as you say. I still think it's irrelevant to view display, including pg_dump, though. Those operations work from a parse tree not a plan tree. regards, tom lane
-
Re: Violation of principle that plan trees are read-only
Jose Luis Tallon <jltallon@adv-solutions.net> — 2025-05-20T21:24:18Z
On 20/5/25 22:43, Nico Williams wrote: > [snip] > What you want is for C to have a type attribute that denotes > immutability all the way down. `const` doesn't do that. One thing that > could be done is to write a utility that creates const-all-the-way-down > clones of given types, but such a tool can't be written as C > pre-processor macros -- it would have to be a tool that parses the > actual type definitions, or uses DWARF or similar to from the > compilation of a file (I've done this latter before, but this weds you > to compilers that output DWARF, which MSVC doesn't, for example). > > Really, the C committee ought to add this at some point, darn it. It > would be the opposite of Rust's &mut. Like C++'s const specifier, specially const references to objects? This is actually natively compatible with C code, "just" by throwing extern "C" around..... https://en.cppreference.com/w/cpp/language/cv (most of Postgres' code is already "Object-oriented in C" in my view...) The fact that the C++ compiler is usually able to optimize deeper/better than a C one due to aggresive inlining+global optimization and inferred "strict"ness doesn't hurt either :) My €.02. HTH. / J.L. -- Parkinson's Law: Work expands to fill the time alloted to it.
-
Re: Violation of principle that plan trees are read-only
Yasir <yasir.hussain.shah@gmail.com> — 2025-05-21T05:01:56Z
On Mon, May 19, 2025 at 7:45 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > Isaac Morland <isaac.morland@gmail.com> writes: > > I assume this question has an obvious negative answer, but why can't we > > attach const declarations to the various structures that make up the plan > > tree (at all levels, all the way down)? I know const doesn't actually > > prevent a value from changing, but at least the compiler would complain > if > > code accidentally tried. > > The big problem is that a "const" attached to a top-level pointer > doesn't inherently propagate down to sub-nodes. So if I had, say, > "const Query *stmt", the compiler would complain about > > stmt->jointree = foo; > > but not about > > stmt->jointree->quals = foo; > > I guess we could imagine developing an entirely parallel set of > struct declarations with "const" on all pointer fields, like > > typedef struct ConstQuery > { > ... > const ConstFromExpr *jointree; > ... > } ConstQuery; > > but even with automated maintenance of the ConstFoo doppelganger > typedefs, it seems like that'd be a notational nightmare. For > one thing, I'm not sure how to teach the compiler that casting > "Query *" to "ConstQuery *" is okay but vice versa isn't. > > Does C++ have a better story in this area? I haven't touched it > in so long that I don't remember. > > regards, tom lane > > One unconventional but potentially effective approach to detect unexpected modifications in the plan tree can be as follows: - Implement a function that can deeply compare two plan trees for structural or semantic differences. - Before passing the original plan tree to the executor, make a deep copy of it. - After execution (or at strategic checkpoints), compare the current plan tree against the original copy. - If any differences are detected, emit a warning or log it for further inspection. Yes, this approach introduces some memory and performance overhead. However, we can limit its impact by enabling it conditionally via a compile-time flag or #define, making it suitable for debugging or assertion-enabled builds. It might sound a bit unconventional, but it could serve as a useful sanity check especially during development or when investigating plan tree integrity issues. Pardon me if this sounds naive! Yasir Data Bene -
Re: Violation of principle that plan trees are read-only
Tom Lane <tgl@sss.pgh.pa.us> — 2025-05-21T16:05:58Z
I wrote: > Oh, scratch that: I'd gotten confused about which branch I was > working in. It does change the output as you say. After poking at that for awhile, I decided we need a small tweak in EXPLAIN itself to make the output consistent. See attached. I'm now leaning against back-patching, as there is a user-visible behavior change in EXPLAIN, and I don't think there are really any severe consequences to the mistaken assignments. regards, tom lane