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

pg_query.proto: Add Token ASCII_36; Add scan test #211

Merged
merged 1 commit into from
Oct 3, 2023

Conversation

evanj
Copy link
Contributor

@evanj evanj commented Sep 30, 2023

Postgres's scanner will scan $ into a single token. For example:

    SELECT $identifier;

The parser will later reject this. However, since the scanner returns this value, we should define a Token for it in the protobuf.

test/scan.c:

  • Add a check so the test does not crash if token_kind is NULL, which it was before I defined the token. The test now prints an error:
    INVALID result for "...": scan_result token 1 token_kind == NULL
  • Make tests constant to prevent accidental modification. Unnecessary but could be helpful.
  • Define testsCount automatically so it no longer needs to be changed.
  • Add an assert to help ensure tests are defined correctly.

Postgres's scanner will scan $ into a single token. For example:

    SELECT $identifier;

The parser will later reject this. However, since the scanner returns
this value, we should define a Token for it in the protobuf.

test/scan.c:

* Add a check so the test does not crash if token_kind is NULL, which
  it was before I defined the token. The test now prints an error:

    INVALID result for "...": scan_result token 1 token_kind == NULL

* Make tests constant to prevent accidental modification. Unnecessary
  but could be helpful.
* Define testsCount automatically so it no longer needs to be changed.
* Add an assert to help ensure tests are defined correctly.
Copy link
Member

@lfittl lfittl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, and thanks for adding tests!

FWIW, whilst it doesn't really make sense to have queries with non-numeric parameter identifiers, it doesn't hurt to support this case since its a valid output of the scan function (which as you note then gets rejected by the parser in regular Postgres).

Out of curiosity, is there a particular use case where you have queries written like this?

@lfittl lfittl merged commit 0281901 into pganalyze:15-latest Oct 3, 2023
8 checks passed
@evanj
Copy link
Contributor Author

evanj commented Oct 3, 2023

As far as I know so far: this $ token is always a parse error. According to my reading of the documentation, Postgres does not permit $ as an operator, but it does permit it:

  • In parameter placeholders: SELECT $5;
  • After the first character of an identifier: CREATE TABLE valid$identifier ();
  • As dollar quote: SELECT $$string with $ lots of stuff$$;
  • As a "named" dollar quote: SELECT $name$my string$name$;

I found this because I wrote a fuzz test for a tool that is rewriting Postgres queries using https://github.com/pganalyze/pg_query_go , and it happened to generate this. The "unsupported" token caused my fuzz test to be unhappy.

So: This isn't "needed", except for completeness for supporting any Postgres tool.

@evanj evanj deleted the evan.jones/dollar-token branch October 3, 2023 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants