3
\$\begingroup\$

I wrote a stored procedure that allows a join entity to be created by name for my fictional characters database. I would like some feedback on it with regard to exception handling, and conditions I didn't think of. Also I would like to know if this procedure is good as far as performance goes.

The tables it involves are characters, medias, actors, character_media_actor. The schemas are as follows.

CREATE TABLE medias (
  med_unv_id       INTEGER NOT NULL REFERENCES universes (unv_id),
  med_title        VARCHAR(255) NOT NULL,
  med_release_date DATE,
  med_type         INTEGER REFERENCES media_types (mt_id),
  med_id           SERIAL PRIMARY KEY,
  med_attribs      JSON
);

CREATE OR REPLACE VIEW v_medias AS
SELECT
  med_unv_id,
  med_title,
  med_release_date,
  mt_name as type,
  med_id,
  med_attribs,
  mt_uses_actors
FROM medias m
  JOIN media_types mt ON mt.mt_id = m.med_type;

CREATE TABLE actors (
  act_id    SERIAL PRIMARY KEY,
  act_name  VARCHAR(255) NOT NULL,
  act_birth DATE,
  act_death DATE,
  act_imdb  VARCHAR(4096) UNIQUE,
  act_bio   TEXT,
  is_locked  BOOLEAN DEFAULT FALSE,
  CHECK (act_birth < act_death)
);
DROP TABLE IF EXISTS characters CASCADE;
CREATE TABLE characters (
  chr_unv_id INTEGER NOT NULL REFERENCES universes (unv_id),
  chr_name   VARCHAR(255) NOT NULL,
  chr_id     SERIAL PRIMARY KEY,
  chr_bio    TEXT,
  UNIQUE(chr_name,chr_unv_id)
);
CREATE TABLE character_media_actors (
  cma_act_id INTEGER REFERENCES actors (act_id),
  cma_chr_id INTEGER NOT NULL REFERENCES characters (chr_id),
  cma_med_id INTEGER NOT NULL REFERENCES medias (med_id),
  cma_id     SERIAL PRIMARY KEY,
  UNIQUE(cma_act_id,cma_chr_id,cma_med_id)
);

The stored procedure is as follows:

CREATE OR REPLACE FUNCTION add_cma_by_names(
  IN p_med_title VARCHAR(255),
  IN p_act_name  VARCHAR(255),
  IN p_chr_name  VARCHAR(255)
)
  RETURNS VOID AS $$
DECLARE
  lk_act_id INTEGER := (SELECT act_id
                        FROM actors
                        WHERE LOWER(act_name) = LOWER(p_act_name));
  lk_chr_id INTEGER := NULL;
  lk_med_id INTEGER := NULL;
  md_unv_id INTEGER := NULL;
  cr_unv_id INTEGER := NULL;
  mt_actors BOOLEAN;
BEGIN
  -- perform validation first
  IF p_med_title IS NULL OR LENGTH(p_med_title) = 0
  THEN
    RAISE EXCEPTION 'A media title must be given';
  END IF;
  IF p_chr_name IS NULL OR LENGTH(p_chr_name) = 0
  THEN
    RAISE EXCEPTION 'A character name must be given';
  END IF;

  -- load the media and universe ids in one go
  SELECT
    med_unv_id,
    med_id,
    mt_uses_actors
  INTO md_unv_id, lk_med_id, mt_actors
  FROM v_medias
  WHERE LOWER(p_med_title) = LOWER(med_title);
  -- we require the universe id before we can get the character id
  SELECT
    chr_id,
    chr_unv_id
  INTO lk_chr_id, cr_unv_id
  FROM characters
  WHERE LOWER(p_chr_name) = LOWER(chr_name);

  IF mt_actors IS TRUE AND lk_act_id IS NULL
  THEN
    RAISE EXCEPTION
    'Selected media has actors must not be null: actor = %', p_act_name;
  END IF;

  IF lk_med_id IS NULL
  THEN
    RAISE EXCEPTION 'Media Not Found, name = %', p_med_title;
  END IF;

  IF lk_chr_id IS NULL
  THEN
    RAISE EXCEPTION 'Character Not Found, name = %', p_chr_name;
  END IF;

  IF md_unv_id <> cr_unv_id
  THEN
    RAISE EXCEPTION 'Character and Media must be in same universe';
  END IF;

  INSERT INTO character_media_actors (cma_act_id, cma_chr_id, cma_med_id) VALUES
    (lk_act_id, lk_chr_id, lk_med_id);

  RETURN;
END $$
LANGUAGE PLPGSQL;
\$\endgroup\$
1
\$\begingroup\$

I'm not a postgres expert by any means, so take this as just an opinion:

DECLARE
  lk_act_id INTEGER := (SELECT act_id
                        FROM actors
                        WHERE LOWER(act_name) = LOWER(p_act_name));

Several issues with this.

I don't think the DECLARE section should be used for querying a table. I think it should only perform literal assignments. (I don't know if there's any accepted guidance around this, but it seems wrong to me.)

LOWER(act_name) will ensure any index on that column isn't used. Granted it's not an issue in your code because you don't have an index, but in the real world you probably would (or you'd suffer the performance impact of a sequential scan on that table every time the function is called, which would become an issue with a large table).

act_name has no guarantee of uniqueness - and it shouldn't, as there can certainly be more than one actor with the same name. So the above would throw an exception if it found more than one result. What then?

The query runs before the validation. If validation subsequently fails, you've done a sequential scan for nothing.

lk_chr_id INTEGER := NULL;
lk_med_id INTEGER := NULL;
md_unv_id INTEGER := NULL;
cr_unv_id INTEGER := NULL;

None of these variables need to be explicitly set to NULL, as all variables not explicitly initialised are NULL by default. (I'm not sure if this holds for all types e.g. enums, but certainly for all "native" and custom types I've used, it holds.)

  IF p_med_title IS NULL OR LENGTH(p_med_title) = 0
  THEN
    RAISE EXCEPTION 'A media title must be given';
  END IF;
  IF p_chr_name IS NULL OR LENGTH(p_chr_name) = 0
  THEN
    RAISE EXCEPTION 'A character name must be given';
  END IF;

What if it's whitespace? I'd do something like this instead:

IF NULLIF(TRIM(p_med_title), '') IS NULL THEN...

Although, I might cache the trimmed inputs so I don't have to trim them again when using them in the subsequent queries.

Also you could use ELSIF rather than having two separate IF statements.

You should probably also validate p_act_name here before looking it up in the table, or if it's valid for this parameter to be null or empty, then check it's not null/empty before querying the table for the id.


As to the rest, I can't quite understand what exactly it's trying to do and what all the conditions are. I would prefer seeing the desired outcome and constraints written in plain English rather than working it out from the code.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.