Compute team review - Githubissues

tristan957 commented 1 week ago

[x] Why can't the extension be relocatable (control file)?
[ ] Are the thread local variables supposed to be per-backend? Doesn't seem like they need to be thread local to me. I think this has been previously discussed.
[x] I don't think a panic is the correct way to do error handling, like in auth::init(), which can only be called once. In postgres, you would use an ereport.
[x] Can we evaluate our unwrap/expect usage, and instead use ereport?
[x] Remove the "Created by pgrx" from the extension control file.
[x] Should we set version to something other than 0.0.0 for the initial inclusion into the compute image?
[x] I don't see any pgrx_embed!(). Is it important?
[x] Can we rename the schema from auth to session_jwt or something? auth is pretty generic and not related to the extension name.
[x] Mind renaming auth.get() to something like auth.get_session()? Anything but a naked get would be good. Looking at auth.user_id() which is a getter, maybe something like auth.session() makes sense.
[x] Can we use a more descriptive comment in the control file, similar to pgvector?

I'll leave the Rust stuff to the experts :). It's possible pgrx is taking the panics and transforming them into Postgres errors, but if it does do that, what error messages does it produce for unwraps, and what SQL error code is produced with it?

davidgomes commented 1 week ago

Are the thread local variables supposed to be per-backend? Doesn't seem like they need to be thread local to me.

I think we're tracking that here: https://github.com/neondatabase/cloud/issues/16041.

Mind renaming auth.get() to something like auth.get_session()

Absolutely, it's now auth.session()

Can we rename the schema from auth tosession_jwt`?

We shouldn't, because developers using this will type it a lot, e.g.:

create table todos (
  id bigint generated by default as identity primary key,
  user_id text not null,
  task text check (char_length(task) > 0),
  is_complete boolean default false,
  inserted_at timestamp with time zone default timezone('utc'::text, now()) not null
);

alter table todos enable row level security;

create policy "Individuals can create todos." on todos for
    insert with check (auth.user_id() = user_id);

create policy "Individuals can view their own todos. " on todos for
    select using (auth.user_id() = user_id);

create policy "Individuals can update their own todos." on todos for
    update using (auth.user_id() = user_id);

create policy "Individuals can delete their own todos." on todos for
    delete using (auth.user_id() = user_id);

I've addressed most everything else in a PR: https://github.com/neondatabase/pg_session_jwt/pull/7.

tristan957 commented 1 week ago

We shouldn't, because developers using this will type it a lot

Then I would rename the extension to something that mentions auth 🤷

davidgomes commented 1 week ago

We shouldn't, because developers using this will type it a lot

Then I would rename the extension to something that mentions auth 🤷

We were initially going with that, but auth is pretty confusing because it can mean authentication or authorization (or both some times).

Ultimately, this isn't very important because this extension will be automatically installed for users and we don't really expect anyone to use it outside of Neon.