Loop in function does not work as expected

Question

Using PostgreSQL 9.0.4

Below is a very similar structure of my table:

CREATE TABLE departamento
(
  id bigserial NOT NULL,
  master_fk bigint,
  nome character varying(100) NOT NULL
  CONSTRAINT departamento_pkey PRIMARY KEY (id),
  CONSTRAINT departamento_master_fk_fkey FOREIGN KEY (master_fk)
      REFERENCES departamento (id) MATCH SIMPLE
      ON UPDATE NO ACTION ON DELETE NO ACTION
)

And the function I created:

CREATE OR REPLACE FUNCTION fn_retornar_dptos_ate_raiz(bigint[])
  RETURNS bigint[] AS
$BODY$
DECLARE
   lista_ini_dptos ALIAS FOR $1;
   dp_row departamento%ROWTYPE;
   dpto bigint;
   retorno_dptos bigint[];
BEGIN
   BEGIN
      PERFORM id FROM tbl_temp_dptos;
      EXCEPTION 
         WHEN undefined_table THEN
            EXECUTE 'CREATE TEMPORARY TABLE tbl_temp_dptos (id bigint NOT NULL) ON COMMIT DELETE ROWS';
   END;

   FOR i IN array_lower(lista_ini_dptos, 1)..array_upper(lista_ini_dptos, 1) LOOP
      SELECT id, master_fk INTO dp_row FROM departamento WHERE id=lista_ini_dptos[i];
      IF dp_row.id IS NOT NULL THEN
         EXECUTE 'INSERT INTO tbl_temp_dptos VALUES ($1)' USING dp_row.id;
         WHILE dp_row.master_fk IS NOT NULL LOOP
            dpto := dp_row.master_fk;
            SELECT id, master_fk INTO dp_row FROM departamento WHERE id=lista_ini_dptos[i];
            EXECUTE 'INSERT INTO tbl_temp_dptos VALUES ($1)' USING dp_row.id;
         END LOOP;
      END IF;
   END LOOP;

   RETURN ARRAY(SELECT id FROM tbl_temp_dptos);
END;
$BODY$
  LANGUAGE plpgsql VOLATILE

Any questions about the names I can translate ..

What is the idea of the function? I first check if the temporary table already exists (perform), and when the exception occurs I create a temporary table.

Then I take each element in the array and use it to fetch the id and master_fk of a department. If the search is successful (check if id is not null, it is even unnecessary) I insert the id in the temporary table and start a new loop.

The second loop is intended to get all parents of that department which was previously found by performing the previous steps (ie, pick a department and insert it into the temporary table).

At the end of the second loop returns to the first. When this one ends I return bigint[] refers to what was recorded in the temporary table.

My problem is that the function returns me the same list I provide. What am I doing wrong?

Answer 1

There is a lot I would do differently, and to great effect.

Table definition

Starting with the table definition and naming conventions. These are mostly just opinions:

CREATE TEMP TABLE conta (conta_id bigint primary key, ...);

CREATE TEMP TABLE departamento (
   dept_id   serial PRIMARY KEY
 , master_id int REFERENCES departamento (dept_id)
 , conta_id  bigint NOT NULL REFERENCES conta (conta_id)
 , nome      text NOT NULL
);

Major points

• Are you sure you need a bigserial for departments? There are hardly that many on this planet. A plain serial should suffice.

• I hardly ever use character varying with a length restriction. Unlike with some other RDBMS there is no performance gain whatsoever by using a restriction. Add a CHECK constraint if you really need to enforce a maximum length. I just use text, mostly and save myself the trouble.

• I suggest a naming convention where the foreign key column shares the name with the referenced column, so master_id instead of master_fk, etc. Also allows to use USING in joins.

• And I rarely use the non-descriptive column name id. Using dept_id instead here.

PL/pgSQL function

It can be largely simplified to:

CREATE OR REPLACE FUNCTION f_retornar_plpgsql(lista_ini_depts VARIADIC int[])
  RETURNS int[] AS
$func$
DECLARE
   _row departamento;                     -- %ROWTYPE is just noise
BEGIN

IF NOT EXISTS (                           -- simpler in 9.1+, see below
    SELECT FROM pg_catalog.pg_class
    WHERE  relnamespace = pg_my_temp_schema()
    AND    relname      = 'tbl_temp_dptos') THEN

   CREATE TEMP TABLE tbl_temp_dptos (dept_id bigint NOT NULL)
   ON COMMIT DELETE ROWS;
END IF;

FOR i IN array_lower(lista_ini_depts, 1)  -- simpler in 9.1+, see below
      .. array_upper(lista_ini_depts, 1) LOOP
   SELECT *  INTO _row                    -- since rowtype is defined, * is best
   FROM   departamento
   WHERE  dept_id = lista_ini_depts[i];

   CONTINUE WHEN NOT FOUND;

   INSERT INTO tbl_temp_dptos VALUES (_row.dept_id);

   LOOP
      SELECT *  INTO _row
      FROM   departamento
      WHERE  dept_id = _row.master_id;

      EXIT WHEN NOT FOUND;

      INSERT INTO tbl_temp_dptos
      SELECT _row.dept_id
      WHERE  NOT EXISTS (
         SELECT FROM tbl_temp_dptos
         WHERE dept_id =_row.dept_id);
   END LOOP;
END LOOP;

RETURN ARRAY(SELECT dept_id FROM tbl_temp_dptos);

END
$func$  LANGUAGE plpgsql;

Call:

SELECT f_retornar_plpgsql(2, 5);

Or:

SELECT f_retornar_plpgsql(VARIADIC '{2,5}');

• ALIAS FOR $1 is outdated syntax and discouraged. Use function parameters instead.

• The VARIADIC parameter makes it more convenient to call. Related:

  • Pass multiple values in single parameter

• You don't need EXECUTE for queries without dynamic elements. Nothing to gain here.

• You don't need exception handling to create a table. Quoting the manual here:

  Tip: A block containing an EXCEPTION clause is significantly more expensive to enter and exit than a block without one. Therefore, don't use EXCEPTION without need.

• Postgres 9.1 or later has CREATE TEMP TABLE IF NOT EXISTS. I use a workaround for 9.0 to conditionally create the temp table.

• Postgres 9.1 also offer FOREACH to loop through an arrays.

All that said, here comes the bummer: you don't need most of this.

SQL function with rCTE

Even in Postgres 9.0, a recursive CTE makes this a whole lot simpler:

CREATE OR REPLACE FUNCTION f_retornar_sql(lista_ini_depts VARIADIC int[])
  RETURNS int[] AS
$func$
WITH RECURSIVE cte AS (
   SELECT dept_id, master_id
   FROM   unnest($1) AS t(dept_id)
   JOIN   departamento USING (dept_id)

   UNION ALL
   SELECT d.dept_id, d.master_id
   FROM   cte
   JOIN   departamento d ON d.dept_id = cte.master_id
   )
SELECT ARRAY(SELECT DISTINCT dept_id FROM cte)    -- distinct values
$func$  LANGUAGE sql;

Same call.

Closely related answer with explanation:

• Tree Structure and Recursion

SQL Fiddle demonstrating both.

Answer 2

I managed to fix my code. At the end of this response is its final form, but if you have any suggestions for improvement are welcome. Here are the changes:

1 - I have provided the essential structure of my table, but in reality it is much bigger. Before master_fk field, there is a field called account_fk, and because of the variable department dp_row%**ROWTYPE** the entire structure of my table is copied to the variable, so if I fill only the first two fields, i.e., id and account_fk, then master_fk that is the third field will be null.

2 - @Nicolas was right, and I ended up using the variable dpto for the second loop. And I had forgotten to fill it inside the loop. Besides using it in the search done within the loop.

3 - I added an if statement to make sure that would not have duplicates in the temporary table.

Correction in the structure of my table:

CREATE TABLE departamento
(
  id bigserial NOT NULL,
  account_fk bigint NOT NULL,
  master_fk bigint,
  nome character varying(100) NOT NULL,
  CONSTRAINT departamento_pkey PRIMARY KEY (id),
  CONSTRAINT departamento_account_fk_fkey FOREIGN KEY (account_fk)
      REFERENCES conta (id) MATCH SIMPLE
      ON UPDATE NO ACTION ON DELETE NO ACTION,
  CONSTRAINT departamento_master_fk_fkey FOREIGN KEY (master_fk)
      REFERENCES departamento (id) MATCH SIMPLE
      ON UPDATE NO ACTION ON DELETE NO ACTION
)

My function as it is now:

CREATE OR REPLACE FUNCTION fn_retornar_dptos_ate_raiz(bigint[]) RETURNS bigint[] AS
$BODY$
DECLARE
   lista_ini_dptos ALIAS FOR $1;
   dp_row departamento%ROWTYPE;
   dpto bigint;
BEGIN
   BEGIN
      PERFORM id FROM tbl_temp_dptos;
      EXCEPTION 
         WHEN undefined_table THEN
            EXECUTE 'CREATE TEMPORARY TABLE tbl_temp_dptos (id bigint NOT NULL) ON COMMIT DELETE ROWS';
   END;

   FOR i IN array_lower(lista_ini_dptos, 1)..array_upper(lista_ini_dptos, 1) LOOP
      SELECT id, conta_fk, master_fk INTO dp_row FROM departamento WHERE id=lista_ini_dptos[i];
      EXECUTE 'INSERT INTO tbl_temp_dptos VALUES ($1)' USING dp_row.id;
      dpto := dp_row.master_fk;
--       RAISE NOTICE 'dp_row: (%); ', dp_row.master_fk;
      WHILE dpto IS NOT NULL LOOP
         SELECT id, conta_fk, master_fk INTO dp_row FROM departamento WHERE id=dpto;
         IF NOT(select exists(select 1 from tbl_temp_dptos where id=dp_row.id limit 1)) THEN
            EXECUTE 'INSERT INTO tbl_temp_dptos VALUES ($1)' USING dp_row.id;
         END IF;
         dpto := dp_row.master_fk;
--   RAISE NOTICE 'dp_row: (%); ', dp_row.master_fk;
      END LOOP;
   END LOOP;

   RETURN ARRAY(SELECT id FROM tbl_temp_dptos);
END;
$BODY$
LANGUAGE plpgsql VOLATILE