volatiletech / sqlboiler

Generate a Go ORM tailored to your database schema.
BSD 3-Clause "New" or "Revised" License
6.73k stars 544 forks source link

Upgrade from 2.7.0 to 2.7.2 causes eager loading in tx to fail #318

Closed tarnowsc closed 6 years ago

tarnowsc commented 6 years ago

If you're having a generation problem please answer these questions before submitting your issue. Thanks!

What version of SQLBoiler are you using (sqlboiler --version)?

2.7.0 and after upgrade to 2.7.2 it fails

If this happened at generation time what was the full SQLBoiler command you used to generate your models? (if not applicable leave blank)

If this happened at runtime what code produced the issue? (if not applicable leave blank)

u := models.Users(
        ctxutil.GetTX(ctx),
        qm.Where(models.UserColumns.ID+"=?", ctxutil.GetUserID(ctx)),
        qm.Load("Agreements"),
    ).OneP()

What is the output of the command above with the -d flag added to it? (Provided you are comfortable sharing this, it contains a blueprint of your schema)

Please provide a relevant database schema so we can replicate your issue (Provided you are comfortable sharing this)

CREATE TABLE agreements (
    id agreement_type PRIMARY KEY,
    link text NOT NULL,
    created_at timestamp with time zone NOT NULL DEFAULT now(),
    updated_at timestamp with time zone NOT NULL DEFAULT now()
);

CREATE TABLE users (
    id uuid DEFAULT uuid_generate_v4() PRIMARY KEY,
    username text NOT NULL,
    email text NOT NULL,
    password_digest text NOT NULL,
    name text NOT NULL DEFAULT '',
    surname text NOT NULL DEFAULT '',
    status user_status NOT NULL DEFAULT 'created',
    age integer,
    weight integer,
    height integer,
    sex user_sex NOT NULL DEFAULT 'unknown',
    created_at timestamp with time zone NOT NULL DEFAULT now(),
    updated_at timestamp with time zone NOT NULL DEFAULT now()
);
CREATE UNIQUE INDEX users_unique_lower_email_idx ON users USING btree (lower(email));

CREATE TABLE agreement_users (
    agreement_id agreement_type NOT NULL REFERENCES agreements(id),
    user_id uuid NOT NULL REFERENCES users(id),
    PRIMARY KEY(agreement_id, user_id)
);

Further information. What did you do, what did you expect?

The error message I got is: models: failed to execute a one query for users: failed to eager load Agreements: failed to eager load agreements: pq: unexpected Parse response 'C' Which probably means that connection was not cleaned properly after previous query. When I do the same query using db object and not tx it works perfectly fine. The db (queries) and app logs do not show anything strange and are equal when build using 2.7.0 and 2.7.2, except the panic stated above.

aarondl commented 6 years ago

Knew exactly where to look from the bug report given that a bug fix we had made recently impacted this area. Thanks for the detailed report.

There were several data types missing from the schema you provided, but I just replaced them with serial where possible and deleted the non-essential columns and it all worked out :)

Should be fixed on master! Because it was such a simple and obviously correct change I didn't bother pushing this to dev. Let me know if you still have problems.

aarondl commented 6 years ago

Also two more things:

  1. This bug didn't exist on v3, better upgrade soon, it's really great! The README in the v3 branch has some "what's new" and "getting started" videos and there's a detailed CHANGELOG.md you can read if you prefer text.
  2. For this particular case eager loading isn't really necessary and is in fact less efficient. I would rewrite your query not to use it here. If it's important the relationship struct hold the resulting Agreements, v3 has the ability to create a relationship struct and you can simply do an assignment to put the Agreements on the u.R.Agreements. This is of course without any context or any clue of what you're doing, so take the advice as such :)

    u, err := models.FindUser(db, ctxutil.GetUserID(ctx))
    // handle error
    
    agreements, err := u.Agreements(db).All()
    // handle error
tarnowsc commented 6 years ago

Many thanks for fast response.

In this particular case the performance gain you mentioned is due to using Find instead of One finalizer? Or there is some dark side of eager loading?

aarondl commented 6 years ago

Find is faster than One because it's a specialized query it avoids the typical query building which does a lot more stuff. It just sprintf's a query.

On top of that Find is a helper to do that exact query, so it's just cleaner and more fit for purpose anyway.

Eager loading does a bit of a different query. It does the same inner join, but it additionally pulls back the ids from the join table in order to fill in the relationship struct. This simply is more data coming back from the database, and we do a lot more reflect and crazy code to get there. It also fills in the relationship structs.

The relationship helper on the other hand does a pretty simple generalized query, not as fast as a sprintf but still better than the craziness in eager loading. Doesn't waste time comparing ids etc to fill in the relationship struct.

Eager loading's performance really comes through when you're loading multiple users, and you want all their agreements. That's the real use case for it. Or if you wanted deeper, like a user and his agreements and the agreement's somethings.

So to answer your question both queries are just a little more efficient the way I showed you. :)

aarondl commented 6 years ago

FYI I'm not sure if you are aware/care but concerning this line:

id uuid DEFAULT uuid_generate_v4() PRIMARY KEY,

The uuid-ossp module documentation says if you want v4 uuids only, that you should consider using pgcrypto module with gen_random_uuid() instead because uuid-ossp is not very well maintained and is getting more difficult to port to different platforms.