Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stack overflow when parsing specific query #9

Closed
zhm opened this issue May 16, 2016 · 6 comments
Closed

Stack overflow when parsing specific query #9

zhm opened this issue May 16, 2016 · 6 comments
Assignees
Labels

Comments

@zhm
Copy link
Contributor

zhm commented May 16, 2016

In the process of attempting to port https://github.com/zhm/pg-query-parser to the newer AST, I encountered an error parsing this statement when running the postgres regression tests:

CREATE FOREIGN TABLE ft1 () SERVER no_server

I also checked the latest ruby parser just to see if it was an issue with the node module and it also fails there.

irb(main):002:0> PgQuery.parse 'CREATE FOREIGN TABLE ft1 () SERVER no_server'
SystemStackError: stack level too deep
    from /usr/local/lib/ruby/gems/2.2.0/gems/pg_query-0.9.2/lib/pg_query/parse.rb:5:in `_raw_parse'
    from /usr/local/lib/ruby/gems/2.2.0/gems/pg_query-0.9.2/lib/pg_query/parse.rb:5:in `parse'
    from (irb):2
    from /usr/local/bin/irb:11:in `<main>'

Note that this isn't a statement I care about, it's just in the Postgres regression folder.

https://github.com/postgres/postgres/blob/master/src/test/regress/sql/foreign_data.sql#L269

@zhm
Copy link
Contributor Author

zhm commented May 16, 2016

Also, that statement should be an error since it's not valid.

@lfittl
Copy link
Member

lfittl commented May 16, 2016

@zhm Thanks for reporting, this does indeed look like a bug - I'll investigate.

@lfittl lfittl added the bug label May 16, 2016
@lfittl lfittl self-assigned this May 16, 2016
@lfittl
Copy link
Member

lfittl commented May 16, 2016

Alright, so the issue comes down to the difference between this (old):

_outCreateForeignTableStmt(StringInfo str, const CreateForeignTableStmt *node)
{
  WRITE_NODE_TYPE("CREATEFOREIGNTABLESTMT");

  _outCreateStmtInfo(str, (const CreateStmt *) node);

  WRITE_STRING_FIELD(servername);
  WRITE_NODE_FIELD(options);
}

https://github.com/lfittl/libpg_query/blob/9.4-latest/patches/01_output_nodes_as_json.patch#L4153

and the new, auto-generated one:

static void
_outCreateForeignTableStmt(StringInfo str, const CreateForeignTableStmt *node)
{
  WRITE_NODE_TYPE("CreateForeignTableStmt");

  WRITE_NODE_FIELD(base);
  WRITE_STRING_FIELD(servername);
  WRITE_NODE_PTR_FIELD(options);
}

Note that WRITE_NODE_FIELD expands into

appendStringInfo(str, "\"" CppAsString(fldname) "\": "); \
_outNode(str, &node->fldname); \
appendStringInfo(str, ", "); \

I think the issue here is that the contained base element does not have the correct node tag (i.e. the node tag for base is the same as the node tag for the CreateForeignTableStmt), and hence we end up in an endless recursive loop.

I'll need to fix this in the generator script and add a test case - I'll see if I can do that tonight.

@zhm
Copy link
Contributor Author

zhm commented May 16, 2016

Thanks for looking into this!

@zhm
Copy link
Contributor Author

zhm commented May 16, 2016

I was able to port the CoffeeScript deparser over to the new V2 JSON.

zhm/pg-query-parser#2

The great news is that this is the only bug I found in the conversion. Over 7000 of the statements in the test suite parse with perfect symmetry. Very nice work on the V2 output @lfittl! 👌

@lfittl lfittl closed this as completed in 122b25f May 17, 2016
@lfittl
Copy link
Member

lfittl commented May 17, 2016

@zhm Yay, awesome work porting the deparser to the new format!

The good thing is, this format is more future-proof (since it closely resembles the in-source structs that Postgres uses), so hopefully you don't have to do this again :)

Fixed the stack overflow issue in 122b25f and released as tag 9.5-1.2.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants