Issue with columns named type with PostgreSQL

[muresan]

Platform

• OS: Linux
• RDBMS: PostgreSQL
• Version: v0.15.6

The issue that I'm seeing is that I need to wrap the keywork type in quotes to have a working run of psqldef. If I use :

  "type"                  TEXT NOT NULL,

everything is working as expected, but I do not have control over the source schema unfortunately so I am curious why this happens and if there is a solution.

--export output

CREATE TABLE "public"."domains" (
    "id" serial NOT NULL,
    "name" character varying(255) NOT NULL CONSTRAINT c_lowercase_name CHECK (name::text = lower(name::text)),
    "master" character varying(128) DEFAULT NULL::character varying,
    "last_check" integer,
    "type" character varying(6) NOT NULL,
    "notified_serial" bigint,
    "account" character varying(40) DEFAULT NULL::character varying,
    PRIMARY KEY ("id")
);

Input SQL

CREATE TABLE domains (
  id                    SERIAL PRIMARY KEY,
  name                  VARCHAR(255) NOT NULL,
  master                VARCHAR(128) DEFAULT NULL,
  last_check            INT DEFAULT NULL,
  type                  TEXT NOT NULL,
  notified_serial       BIGINT DEFAULT NULL,
  account               VARCHAR(40) DEFAULT NULL,
  options               TEXT DEFAULT NULL,
  catalog               TEXT DEFAULT NULL,
  CONSTRAINT c_lowercase_name CHECK (((name)::TEXT = LOWER((name)::TEXT)))
);

Current output

found syntax error when parsing DDL "CREATE TABLE domains (
  id                    SERIAL PRIMARY KEY,
  name                  VARCHAR(255) NOT NULL,
  master                VARCHAR(128) DEFAULT NULL,
  last_check            INT DEFAULT NULL,
  type                  TEXT NOT NULL,
  notified_serial       BIGINT DEFAULT NULL,
  account               VARCHAR(40) DEFAULT NULL,
  options               TEXT DEFAULT NULL,
  catalog               TEXT DEFAULT NULL,
  CONSTRAINT c_lowercase_name CHECK (((name)::TEXT = LOWER((name)::TEXT)))
)": syntax error at position 214 near 'type'

Expected output

-- dry run --
ALTER TABLE "public"."domains" ALTER COLUMN "type" TYPE text;

[k0kubun]

I am curious why this happens and if there is a solution.

It's just that the current parser of psqldef is still half-baked. It does have a better situation than other sqldef variants though. psqldef uses a hybrid parser system; it tries to parse SQL with PostgreSQL's original parser and convert it to sqldef AST, or just parse SQL with sqldef's original parser if the conversion is not supported in the first path. The plan is to handle everything in the first path, but it still fallbacks to the old parser in many cases, including this, and the old one doesn't handle this case well.

If you remove some features that are not supported by the new conversion, type works without quotes. For example, it parses CREATE TABLE domains (type TEXT NOT NULL); just fine. So it doesn't seem to support converting features used in other columns or constraints.

The logic lives in database/postgres/parser.go. Would you be interested in filing a pull request for it?

[muresan]

The logic lives in database/postgres/parser.go. Would you be interested in filing a pull request for it?

I am interested however my go skills are very low. I traced the issue to: https://github.com/k0kubun/sqldef/blob/master/database/postgres/parser.go#L489 which only implements "NOT NULL" basically and the new parser fails on basically every other check:

{ "CONSTR_CHECK", 6 },
{ "CONSTR_DEFAULT", 3 },
{ "CONSTR_PRIMARY", 7 },

I imagine the other cases need to be added to the switch but I have no idea how to continue, if you could give me some pointers or some previous commits that would point me to the right direction, I would appreciate it.

[k0kubun]

What I would do is:

1. Start docker-compose up locally.
2. Run psql -U postgres -h 127.0.0.1. Execute create database test; on it.
3. Write an SQL to test as /tmp/a.sql
4. Run go run cmd/psqldef/psqldef.go -U postgres -h 127.0.0.1 test --dry-run < /tmp/a.sql
5. Insert pp.Println(...) to inspect values you're interested in, using https://github.com/k0kubun/pp. That way, you could figure out what will come to that place instead of ConstrType_CONSTR_NOTNULL for your case.

Does that make sense?

[muresan]

What I would do is:

1. Start docker-compose up locally.
2. Run psql -U postgres -h 127.0.0.1. Execute create database test; on it.
3. Write an SQL to test as /tmp/a.sql
4. Run go run cmd/psqldef/psqldef.go -U postgres -h 127.0.0.1 test --dry-run < /tmp/a.sql
5. Insert pp.Println(...) to inspect values you're interested in, using https://github.com/k0kubun/pp. That way, you could figure out what will come to that place instead of ConstrType_CONSTR_NOTNULL for your case.

I did exactly that, using q.Q instead, same outcome, that's how I saw that the parser was not able to parse:

CREATE TABLE domains (
  id                    SERIAL PRIMARY KEY,
  name                  VARCHAR(255) NOT NULL CONSTRAINT c_lowercase_name CHECK (((name)::text = lower((name)::text))),
  master                VARCHAR(128) DEFAULT NULL,
  last_check            INT DEFAULT NULL,
  type                  TEXT NOT NULL,
  notified_serial       BIGINT DEFAULT NULL,
  account               VARCHAR(40) DEFAULT NULL,
  options               TEXT DEFAULT NULL,
  catalog               TEXT DEFAULT NULL
);

PRIMARY KEY , CONSTRAINT ... CHECK, DEFAULT fields. Which I pointed above as

{ "CONSTR_CHECK", 6 },
{ "CONSTR_DEFAULT", 3 },
{ "CONSTR_PRIMARY", 7 },

basically the new parser would fail with: unhandled contype: 7, then 3, then 6. I'm stuck on what to do next. Here is constraint object value for DEFAULT NULL:

0.000s constraint: 
       &pg_query.Constraint{
           state:         impl.MessageState{},
           sizeCache:     0,
           unknownFields: nil,
           Contype:       3,
           Conname:       "",
           Deferrable:    false,
           Initdeferred:  false,
           Location:      139,
           IsNoInherit:   false,
           RawExpr:       &pg_query.Node{
               state:         impl.MessageState{},
               sizeCache:     0,
               unknownFields: nil,
               Node:          &pg_query.Node_AConst{
                   AConst: &pg_query.A_Const{
                       state:         impl.MessageState{},
                       sizeCache:     0,
                       unknownFields: nil,
                       Val:           &pg_query.Node{
                           state:         impl.MessageState{},
                           sizeCache:     0,
                           unknownFields: nil,
                           Node:          &pg_query.Node_Null{
                               Null: &pg_query.Null{},
                           },
                       },
                       Location: 147,
                   },
               },
           },
           CookedExpr:         "",
           GeneratedWhen:      "",
           Keys:               nil,
           Including:          nil,
           Exclusions:         nil,
           Options:            nil,
           Indexname:          "",
           Indexspace:         "",
           ResetDefaultTblspc: false,
           AccessMethod:       "",
           WhereClause:        (*pg_query.Node)(nil),
           Pktable:            (*pg_query.RangeVar)(nil),
           FkAttrs:            nil,
           PkAttrs:            nil,
           FkMatchtype:        "",
           FkUpdAction:        "",
           FkDelAction:        "",
           OldConpfeqop:       nil,
           OldPktableOid:      0x0,
           SkipValidation:     false,
           InitiallyValid:     false,
       }

and here is the same object with DEFAULT 'abc':

           RawExpr:       &pg_query.Node{
               state:         impl.MessageState{},
               sizeCache:     0,
               unknownFields: nil,
               Node:          &pg_query.Node_AConst{
                   AConst: &pg_query.A_Const{
                       state:         impl.MessageState{},
                       sizeCache:     0,
                       unknownFields: nil,
                       Val:           &pg_query.Node{
                           state:         impl.MessageState{},
                           sizeCache:     0,
                           unknownFields: nil,
                           Node:          &pg_query.Node_String_{
                               String_: &pg_query.String{
                                   state:         impl.MessageState{},
                                   sizeCache:     0,
                                   unknownFields: nil,
                                   Str:           "abc",
                               },
                           },
                       },
                       Location: 147,
                   },
               },
           },

Does that make sense?

I got to the part where I need something like:

                case pgquery.ConstrType_CONSTR_DEFAULT:
                        columnType.Default = // here parse the value from constraint then fill in DefaultDefinition
                                              // which seems to be handled already in parseExpr ?

My Go skill stops here, maybe I can revisit at a future time when I have more skill :)

[k0kubun]

basically the new parser would fail with: unhandled contype: 7, then 3, then 6.
I got to the part where I need something like:
My Go skill stops here, maybe I can revisit at a future time when I have more skill :)

The next step is to add case for 7, 3, and 6. If you set up an editor with working gopls, starting to type case pgquery.ConstrType_CONSTR_... would auto-complete something for you. Because you already mentioned "CONSTR_CHECK", "CONSTR_DEFAULT", and "CONSTR_PRIMARY", they would likely end with them respectively. Alternatively, you could clone pganalyze/pg_query_go and grep whatever you want.

Next, you need to fill out those case branches, which I now realized that I didn't cover. What I would do next is:

1. Write an SQL that is a subset of what you want to parse and the legacy parser can also parse. Use it for testing.
   • I would tackle each constr one by one, not all at once, e.g. targeting only PRIMARY first.
2. Dump what the legacy parser gives with q.Q. That should give you what you need to return.
3. Just copy-paste the q.Q's output to the case branch. While keeping the hard-coded string/integer literals as is, make it a valid Go expression.
4. Replace static string/integer literals with variables or appropriate function calls that delegate conversion of sub-nodes. Keep repeating this until all hard-coded values are gone.

This iteration should let the new parser be able to parse what the legacy parser can already parse. It would leave only a tiny little piece that the legacy parser can't parse. At that point, hopefully, you understand what the new parser needs to return. If not, please make that state a pull request, and I can give another comment from there.

[hokaccha]

Probably, I faced the same issue as this one.

CREATE TABLE tbl (value text);
ALTER TABLE tbl ADD CONSTRAINT tbl_value_idx UNIQUE (value);

$ psqldef --dry-run --host localhost --user postgres sandbox < schema.sql
found syntax error when parsing DDL "ALTER TABLE tbl ADD CONSTRAINT tbl_value_idx UNIQUE (value)": syntax error at position 59 near 'value'

It seems to cause an error when a column with the same name as the keywords defined in token.go is specified in alter table.

I will try to parse alter table with the new parser.