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 v10 GENERATED BY DEFAULT AS IDENTITY result in Not-NULL violation #2508

Closed
dekc opened this issue Jul 11, 2019 · 6 comments
Closed

PG v10 GENERATED BY DEFAULT AS IDENTITY result in Not-NULL violation #2508

dekc opened this issue Jul 11, 2019 · 6 comments
Assignees
Labels
c/server Related to server e/easy can be wrapped up in a couple of days k/bug Something isn't working

Comments

@dekc
Copy link

dekc commented Jul 11, 2019

To reproduce:
0. Important: make sure to run this mutation as anonymous (ie not as hasura admin etc). Set "anonymous" permisions on insert and select actions for example (as shown in the link below)

https://media.discordapp.net/attachments/428469959530643466/598763216192929792/unknown.png?width=1321&height=720

  1. Create following table:
CREATE TABLE color (
    color_id INT GENERATED BY DEFAULT AS IDENTITY,
    color_name VARCHAR NOT NULL
);
  1. Run following mutation:
 mutation colorInsert{
  insert_color(objects: {color_name: "green"}){
    returning {
      color_id
      color_name
    }
  }
}

You should the see the following result:

{
  "errors": [
    {
      "extensions": {
        "path": "$.selectionSet.insert_color.args.objects",
        "code": "constraint-violation"
      },
      "message": "Not-NULL violation. null value in column "color_id" violates not-null constraint"
    }
  ]
}

Further info:

  • Hasura running ontop of a v10 PG database
  • Hasura v1.0.0-beta.2
  • Note: that it works if x-hasura-admin-secret header is passed in console headers
@lexi-lambda lexi-lambda added c/server Related to server k/bug Something isn't working labels Jul 11, 2019
@lexi-lambda
Copy link
Contributor

lexi-lambda commented Jul 11, 2019

Interesting bug—I wasn’t aware of Postgres 10’s identity columns. Brief investigation reveals this is more subtle than it might seem. For inserting into tables as non-admin roles, Hasura uses a view. So a view like

CREATE VIEW hdb_views.anonymous__insert__public__color AS SELECT * FROM color;

will be created. For inserts into that view, Hasura uses a trigger, created like

CREATE FUNCTION hdb_views.anonymous__insert__public__color() RETURNS trigger
LANGUAGE plpgsql AS $$ /* ... */ $$;

CREATE TRIGGER anonymous__insert__public__color
INSTEAD OF INSERT ON hdb_views.anonymous__insert__public__color
FOR EACH ROW EXECUTE PROCEDURE hdb_views.anonymous__insert__public__color();

which redirections insertions into the actual color table. In order to handle default values, Hasura copies the defaults from the original table into the view, so if color_id was given type serial, Hasura will also issue the following statement:

ALTER VIEW hdb_views.anonymous__insert__public__color
ALTER COLUMN color_id SET DEFAULT nextval('color_color_id_seq'::regclass);

That works out okay, since views are allowed to have default values. But columns created with GENERATED BY DEFAULT AS IDENTITY don’t actually have default values in the usual sense—information_schema.columns.column_default is NULL for those columns, for example. Instead, they are special “identity” columns that work their own way, through a separate path. And unfortunately, if you insert into a view and don’t specify a value, all you get to see in an INSTEAD OF INSERT trigger is NULL, which isn’t super helpful.

So I think this is a bug, but I’m not immediately sure what the right fix is. Can you get away with using serial columns for now instead of identity columns?

@susen4517x
Copy link

did this bug fixed?

@lexi-lambda lexi-lambda added the e/easy can be wrapped up in a couple of days label Nov 27, 2019
@lexi-lambda
Copy link
Contributor

@susen4517x Sorry for the delayed response. This bug has not been fixed, but it is not forgotten—we’re going to try and fix it sometime in the next few weeks.

@coco98 coco98 assigned paf31 and unassigned dsandip Dec 2, 2019
@lexi-lambda
Copy link
Contributor

@paf31 I did a little bit of extra looking into this that didn’t make it into the issue, and I have a work-in-progress commit from way back in July here: lexi-lambda@fcb3e9f. Here’s what I found:

  1. Getting the default values for identity columns is a little involved, so the hdb_catalog.hdb_columns_identity_default_values view created by the migration in that commit includes the SQL needed to fetch them.

  2. We currently use a Postgres function named hdb_catalog.inject_table_defaults to copy default values from source tables onto the views created from them.

  3. The bulk of the complexity in that commit seems to have been around trying to support identity columns while preserving backwards-compatibility with Postgres 9.5/9.6, but looking at it again, I’m not sure if that’s actually necessary. It looks to me like hdb_catalog.hdb_columns_identity_default_values should still work on Postgres <10, it will just return no rows (since the depend.deptype = 'i' condition will never be true), which is fine. So all the nonsense I was doing with hdb_columns_default_values and modifying it upon migration to Postgres 10 seems like probably a waste of time; you should be able to do something significantly simpler.

So I think you ought to be able to combine the queries in my definitions of hdb_columns_ordinary_default_values and hdb_catalog.hdb_columns_identity_default_values to create a single view, then use that in hdb_catalog.inject_table_defaults, which might be enough on its own to get everything working.

@paf31
Copy link
Contributor

paf31 commented Jan 16, 2020

This should be fixed by the hdb_views change, can this be closed now?

@lexi-lambda
Copy link
Contributor

Fixed by #3598.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c/server Related to server e/easy can be wrapped up in a couple of days k/bug Something isn't working
Projects
None yet
Development

No branches or pull requests

5 participants