BUG #14153: Unrecognized node type error when upsert is present in recursive CTE
The following bug has been logged on the website:
Bug reference: 14153
Logged by: Thomas Alton
Email address: thomas.alton@gmail.com
PostgreSQL version: 9.5.3
Operating system: Ubuntu 14.04.4 LTS
Description:
Simple repro steps that results in an "ERROR: unrecognized node type:
920"
CREATE TABLE foobar (
id TEXT PRIMARY KEY
);
WITH RECURSIVE upserted AS (
INSERT INTO foobar (id) VALUES ('a')
ON CONFLICT (id) DO NOTHING
RETURNING id
)
SELECT id from upserted;
In action:
postgres@postgres:/home/moatra$ psql
psql (9.5.3)
Type "help" for help.
postgres=# SELECT version();
version
-------------------------------------------------------------------------------------------------
PostgreSQL 9.5.3 on x86_64-pc-linux-gnu, compiled by gcc (Ubuntu
4.8.2-19ubuntu1) 4.8.2, 64-bit
(1 row)
postgres=# CREATE DATABASE example;
CREATE DATABASE
postgres=# \c example
You are now connected to database "example" as user "postgres".
example=# CREATE TABLE foobar (
example(# id TEXT PRIMARY KEY
example(# );
CREATE TABLE
example=#
example=# \set VERBOSITY verbose
example=#
example=# WITH RECURSIVE upserted AS (
example(# INSERT INTO foobar (id) VALUES ('a')
example(# ON CONFLICT (id) DO NOTHING
example(# RETURNING id
example(# )
example-# SELECT id FROM upserted;
ERROR: XX000: unrecognized node type: 920
LOCATION: raw_expression_tree_walker, nodeFuncs.c:3410
I expected the query run successfully and return one row with 'a'.
There doesn't even need to be a recursive query in the CTE, just the
RECURSIVE keyword. Removing the "ON CONFLICT ..." clause results in a
success.
example=# WITH RECURSIVE upserted AS (
example(# INSERT INTO foobar (id) VALUES ('a')
example(# RETURNING id
example(# )
example-# SELECT id FROM upserted;
id
----
a
(1 row)
Removing the "RECURSIVE" keyword and leaving the "ON CONFLICT ..." clause is
also successful.
example=# WITH upserted AS (
example(# INSERT INTO foobar (id) VALUES ('a'), ('b')
example(# ON CONFLICT (id) DO NOTHING
example(# RETURNING id
example(# )
example-# SELECT id FROM upserted;
id
----
b
(1 row)
The server was installed by adding the
http://apt.postgresql.org/pub/repos/apt/ repo and using apt for
installation. Originally I installed 9.5.2, but upgraded to 9.5.3 to
re-verify this bug on the latest release. Running uname -a gives:
moatra@postgres:~$ uname -a
Linux postgres 3.13.0-85-generic #129-Ubuntu SMP Thu Mar 17 20:50:15 UTC
2016 x86_64 x86_64 x86_64 GNU/Linux
Please let me know if there's anything else I can do to be of assistance.
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
On Sat, May 21, 2016 at 4:28 PM, <thomas.alton@gmail.com> wrote:
ERROR: XX000: unrecognized node type: 920
LOCATION: raw_expression_tree_walker, nodeFuncs.c:3410I expected the query run successfully and return one row with 'a'.
This is clearly a bug.
920 is IndexElem, which can appear in a raw parse tree in 9.5, due to
ON CONFLICT's use of inference reusing what was previously only used
for CREATE INDEX. (It does not make it into the post-parse analysis
tree, though).
RECURSIVE isn't really special. You don't see the bug in the
non-RECURSIVE case because of the differing rules on query name
visibility.
I'll add a fix for this bug to my personal TODO list. It shouldn't be
hard to fix.
--
Peter Geoghegan
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
On Sat, May 21, 2016 at 5:03 PM, Peter Geoghegan <pg@heroku.com> wrote:
On Sat, May 21, 2016 at 4:28 PM, <thomas.alton@gmail.com> wrote:
ERROR: XX000: unrecognized node type: 920
LOCATION: raw_expression_tree_walker, nodeFuncs.c:3410I expected the query run successfully and return one row with 'a'.
This is clearly a bug.
920 is IndexElem, which can appear in a raw parse tree in 9.5, due to
ON CONFLICT's use of inference reusing what was previously only used
for CREATE INDEX. (It does not make it into the post-parse analysis
tree, though).
Attached patch fixes this issue by adding the appropriate
raw_expression_tree_walker handler. This isn't the first quasi-utility
node to need such a handler, so the fix is simple.
--
Peter Geoghegan
Attachments:
0001-Add-IndexElem-to-raw_expression_tree_walker.patchtext/x-patch; charset=US-ASCII; name=0001-Add-IndexElem-to-raw_expression_tree_walker.patchDownload+12-1
Peter Geoghegan <pg@heroku.com> writes:
Attached patch fixes this issue by adding the appropriate
raw_expression_tree_walker handler. This isn't the first quasi-utility
node to need such a handler, so the fix is simple.
It seems unlikely to me that recursing into the name lists is helpful
here: those are not going to contain any data that is interpretable
without context. Did you have a reason to do that?
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, May 23, 2016 at 12:22 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
It seems unlikely to me that recursing into the name lists is helpful
here: those are not going to contain any data that is interpretable
without context. Did you have a reason to do that?
I saw no reason to avoid the extra cycles. A noticeable omission has a
cost: it gets noticed. Given this code path is likely to hardly ever
be hit, this mechanical approach seemed best. That's all.
--
Peter Geoghegan
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Peter Geoghegan <pg@heroku.com> writes:
On Mon, May 23, 2016 at 12:22 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
It seems unlikely to me that recursing into the name lists is helpful
here: those are not going to contain any data that is interpretable
without context. Did you have a reason to do that?
I saw no reason to avoid the extra cycles. A noticeable omission has a
cost: it gets noticed. Given this code path is likely to hardly ever
be hit, this mechanical approach seemed best. That's all.
I agree that performance isn't much of a concern, but code bloat and
inconsistency with other cases are valid concerns. That function does
not recurse into name lists in, for example, the A_Expr and FuncCall
cases.
Also, related to this complaint though not this patch, it's disturbing
that this oversight wasn't detected long ago. My first thought was to add
some conditionally-compiled debugging code that just performs a no-op
traverse of every raw parse tree produced by the grammar. However that
doesn't work because raw_expression_tree_walker doesn't promise to support
everything, only DML statements. Thoughts?
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, May 23, 2016 at 12:48 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I saw no reason to avoid the extra cycles. A noticeable omission has a
cost: it gets noticed. Given this code path is likely to hardly ever
be hit, this mechanical approach seemed best. That's all.I agree that performance isn't much of a concern, but code bloat and
inconsistency with other cases are valid concerns. That function does
not recurse into name lists in, for example, the A_Expr and FuncCall
cases.
Okay. Noted.
Also, related to this complaint though not this patch, it's disturbing
that this oversight wasn't detected long ago. My first thought was to add
some conditionally-compiled debugging code that just performs a no-op
traverse of every raw parse tree produced by the grammar. However that
doesn't work because raw_expression_tree_walker doesn't promise to support
everything, only DML statements. Thoughts?
Why couldn't the debug code be executed conditionally, too? Since
raw_expression_tree_walker() promises to work for "SelectStmt and
everything that could appear under it", I don't think it would be
difficult to find a choke point for that. Perhaps there is some
subtlety I've missed, since what I propose seems too obvious. FWIW, I
don't think it would much matter if this debug code was not reliably
executed for every possible SelectStmt. Just limiting it to top-level
SelectStmts would have easily caught this bug.
--
Peter Geoghegan
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Peter Geoghegan <pg@heroku.com> writes:
On Mon, May 23, 2016 at 12:48 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Also, related to this complaint though not this patch, it's disturbing
that this oversight wasn't detected long ago. My first thought was to add
some conditionally-compiled debugging code that just performs a no-op
traverse of every raw parse tree produced by the grammar. However that
doesn't work because raw_expression_tree_walker doesn't promise to support
everything, only DML statements. Thoughts?
Why couldn't the debug code be executed conditionally, too? Since
raw_expression_tree_walker() promises to work for "SelectStmt and
everything that could appear under it", I don't think it would be
difficult to find a choke point for that. Perhaps there is some
subtlety I've missed, since what I propose seems too obvious. FWIW, I
don't think it would much matter if this debug code was not reliably
executed for every possible SelectStmt. Just limiting it to top-level
SelectStmts would have easily caught this bug.
Um, I think not --- you need the case cited by the OP, namely an INSERT
ON CONFLICT in a CTE in a SelectStmt. If we'd had any of those in the
regression tests, we'd have found the bug, but we don't; and it's not
an obvious combination to try. We would have found it if there were
a reason to run raw_expression_tree_walker() on bare InsertStmts,
but currently there is not.
Possibly we could get adequate coverage by inserting the debug code
into the first four switch cases in transformStmt().
If that sounds like a plausible choke-point, the next question is what
to use to enable the debug test. I propose "#ifdef COPY_PARSE_PLAN_TREES"
since that enables similar sanity checking for other parts of
backend/nodes/, and we do have at least one buildfarm member using it.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, May 23, 2016 at 1:55 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Um, I think not --- you need the case cited by the OP, namely an INSERT
ON CONFLICT in a CTE in a SelectStmt. If we'd had any of those in the
regression tests, we'd have found the bug, but we don't; and it's not
an obvious combination to try. We would have found it if there were
a reason to run raw_expression_tree_walker() on bare InsertStmts,
but currently there is not.
I don't follow. If we had coverage of that in the regression tests,
then that would have shown the bug, of course. It would have shown the
bug without any new debugging infrastructure being required (or being
enabled).
What I meant is that I think naively adding the choke-point for a
top-level SelectStmt would do the job (although perhaps we could do
better). I wasn't suggesting that we'd avoid recursing from there;
only that we could reasonably avoid recursing from someplace else
(e.g. InsertStmt) in the hope of finding a SelectStmt to test.
Offhand, I don't imagine that that would offer better coverage.
If that sounds like a plausible choke-point, the next question is what
to use to enable the debug test. I propose "#ifdef COPY_PARSE_PLAN_TREES"
since that enables similar sanity checking for other parts of
backend/nodes/, and we do have at least one buildfarm member using it.
That's what I was thinking, too. No need to keep it separate.
--
Peter Geoghegan
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Peter Geoghegan <pg@heroku.com> writes:
What I meant is that I think naively adding the choke-point for a
top-level SelectStmt would do the job (although perhaps we could do
better). I wasn't suggesting that we'd avoid recursing from there;
only that we could reasonably avoid recursing from someplace else
(e.g. InsertStmt) in the hope of finding a SelectStmt to test.
Offhand, I don't imagine that that would offer better coverage.
I think it would. The attached patch shows nearly 150 failures in
the current regression tests. If I remove "case T_InsertStmt" that
drops to two failures: there are two cases in with.sgml that almost
manage to exercise the bug, but not quite because they are not
WITH RECURSIVE. That doesn't leave me with any warm feeling about
being able to find other similar oversights if we apply this testing
only to top-level SelectStmts.
If that sounds like a plausible choke-point, the next question is what
to use to enable the debug test. I propose "#ifdef COPY_PARSE_PLAN_TREES"
since that enables similar sanity checking for other parts of
backend/nodes/, and we do have at least one buildfarm member using it.
That's what I was thinking, too. No need to keep it separate.
After cogitating, I did it as attached just for readability's sake.
regards, tom lane
Attachments:
raw-expression-coverage-test.patchtext/x-diff; charset=us-ascii; name=raw-expression-coverage-test.patchDownload+55-6
I wrote:
Peter Geoghegan <pg@heroku.com> writes:
If that sounds like a plausible choke-point, the next question is what
to use to enable the debug test. I propose "#ifdef COPY_PARSE_PLAN_TREES"
since that enables similar sanity checking for other parts of
backend/nodes/, and we do have at least one buildfarm member using it.
That's what I was thinking, too. No need to keep it separate.
After cogitating, I did it as attached just for readability's sake.
And after further thought, I decided that that was penny-wise and
pound-foolish; it's more readable if the #define is just an independent
pg_config_manual.h entry. The only work it'd save is the need to update
a buildfarm animal or two to add the new #define, which is not exactly
a huge cost.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, May 23, 2016 at 4:11 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
And after further thought, I decided that that was penny-wise and
pound-foolish; it's more readable if the #define is just an independent
pg_config_manual.h entry. The only work it'd save is the need to update
a buildfarm animal or two to add the new #define, which is not exactly
a huge cost.
You also have to be aware of the new thing, which a lot of hackers
will not be, if awareness of COPY_PARSE_PLAN_TREES is anything to go
by.
--
Peter Geoghegan
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Peter Geoghegan <pg@heroku.com> writes:
On Mon, May 23, 2016 at 4:11 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
And after further thought, I decided that that was penny-wise and
pound-foolish; it's more readable if the #define is just an independent
pg_config_manual.h entry. The only work it'd save is the need to update
a buildfarm animal or two to add the new #define, which is not exactly
a huge cost.
You also have to be aware of the new thing, which a lot of hackers
will not be, if awareness of COPY_PARSE_PLAN_TREES is anything to go
by.
I doubt that anybody ever enables that in manual builds anyway.
The important thing is to have at least one buildfarm animal using it,
which there soon will be.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, May 23, 2016 at 4:27 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
You also have to be aware of the new thing, which a lot of hackers
will not be, if awareness of COPY_PARSE_PLAN_TREES is anything to go
by.I doubt that anybody ever enables that in manual builds anyway.
The important thing is to have at least one buildfarm animal using it,
which there soon will be.
I manually enable it sometimes. And, I've pointed out bugs that that
would have caught to other hackers during review a couple of times.
That's my view. I'm not going to make a fuss about it.
--
Peter Geoghegan
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers