0

I am trying to create a function and I can't find my error in the following code:

CREATE OR REPLACE FUNCTION qwat_od.fn_label_create_fields(table_name varchar, position boolean = true, rotation boolean = true) RETURNS void AS
$BODY$
    BEGIN
        /* Creates columns */
        EXECUTE 'ALTER TABLE qwat_od.'||table_name||' ADD COLUMN label_1_visible  smallint default 1;           ';
        IF position IS TRUE THEN
            EXECUTE 'ALTER TABLE qwat_od.'||table_name||' ADD COLUMN label_1_x        double precision default null;';
            EXECUTE 'ALTER TABLE qwat_od.'||table_name||' ADD COLUMN label_1_y        double precision default null;';
        END IF;
        IF rotation IS TRUE THEN
            EXECUTE 'ALTER TABLE qwat_od.'||table_name||' ADD COLUMN label_1_rotation double precision default null;';
        END IF;
        EXECUTE 'ALTER TABLE qwat_od.'||table_name||' ADD COLUMN label_1_text     varchar(120);';

        EXECUTE 'ALTER TABLE qwat_od.'||table_name||' ADD COLUMN label_2_visible  smallint default 1;           ';
        IF position IS TRUE THEN
            EXECUTE 'ALTER TABLE qwat_od.'||table_name||' ADD COLUMN label_2_x        double precision default null;';
            EXECUTE 'ALTER TABLE qwat_od.'||table_name||' ADD COLUMN label_2_y        double precision default null;';
        END IF;
        IF rotation IS TRUE THEN
            EXECUTE 'ALTER TABLE qwat_od.'||table_name||' ADD COLUMN label_2_rotation double precision default null;';
        END IF;

        EXECUTE 'ALTER TABLE qwat_od.'||table_name||' ADD COLUMN label_2_text     varchar(120);';
        /* Creates constraints */
        EXECUTE 'ALTER TABLE qwat_od.'||table_name||' ADD CONSTRAINT '||table_name||'_label_1_visible FOREIGN KEY (label_1_visible) REFERENCES qwat_vl.visible(vl_code_int) MATCH FULL; CREATE INDEX fki_'||table_name||'_label_1_visible  ON qwat_od.'||table_name||'(label_1_visible);';
        EXECUTE 'ALTER TABLE qwat_od.'||table_name||' ADD CONSTRAINT '||table_name||'_label_2_visible FOREIGN KEY (label_2_visible) REFERENCES qwat_vl.visible(vl_code_int) MATCH FULL; CREATE INDEX fki_'||table_name||'_label_2_visible  ON qwat_od.'||table_name||'(label_2_visible);';
    END;
$BODY$
LANGUAGE 'plpgsql';

I get this:

ERROR:  syntax error at or near "position"
LINE 4: ...wat_od.fn_label_create_fields(table_name varchar, position b...

Did I do something wrong in the declaration of arguments?

Erwin Brandstetter
  • 605,456
  • 145
  • 1,078
  • 1,228
Denis Rouzaud
  • 2,412
  • 2
  • 26
  • 45
  • @a_horse_with_no_name it supports (basically it supports any sql expressions http://www.postgresql.org/docs/current/static/plpgsql-expressions.html ) -- but of course `IF THEN` is way more understandable. – pozs Jan 26 '15 at 10:08
  • @a_horse_with_no_name: [this documentation page](http://www.postgresql.org/docs/9.1/static/functions-comparison.html) suggests that `is true` is supported – Andomar Jan 26 '15 at 10:13
  • 4
    `position` is an SQL reserved word, you should quote it http://www.postgresql.org/docs/current/static/sql-keywords-appendix.html – pozs Jan 26 '15 at 10:13
  • Thanks!!! It was indeed the position which caused troubles. – Denis Rouzaud Jan 26 '15 at 12:49

1 Answers1

2

@pozs already provided an explanation for the error you saw.
But there is more. Most importantly, you are wide open to SQL injection.

CREATE OR REPLACE FUNCTION qwat_od.fn_label_create_fields(_tbl text, _position bool = true, _rotation bool = true)
  RETURNS void AS
$func$
BEGIN
    /* Creates columns */
   EXECUTE format('ALTER TABLE qwat_od.%I ADD COLUMN label_1_visible smallint default 1', _tbl);

   IF _position THEN
       EXECUTE format('ALTER TABLE qwat_od.%I
           ADD COLUMN label_1_x double precision default null
         , ADD COLUMN label_1_y double precision default null', _tbl);
   END IF;
   IF _rotation THEN
       EXECUTE format('ALTER TABLE qwat_od.%I ADD COLUMN label_1_rotation double precision default null' , _tbl);
   END IF;

   EXECUTE format('ALTER TABLE qwat_od.%I
           ADD COLUMN label_1_text varchar(120)    
         , ADD COLUMN label_2_visible  smallint default 1', _tbl);

   IF _position THEN
       EXECUTE format('ALTER TABLE qwat_od.%I
           ADD COLUMN label_2_x double precision default null
         , ADD COLUMN label_2_y double precision default null', _tbl);
   END IF;
   IF _rotation THEN
       EXECUTE format('ALTER TABLE qwat_od.%I ADD COLUMN label_2_rotation double precision default null', _tbl);
   END IF;

   EXECUTE format('ALTER TABLE qwat_od.%I ADD COLUMN label_2_text varchar(120)', _tbl);

   /* Creates constraints */
   EXECUTE format('ALTER TABLE qwat_od.%$1I
           ADD CONSTRAINT %$2I FOREIGN KEY (label_1_visible) REFERENCES qwat_vl.visible(vl_code_int)
         , ADD CONSTRAINT %$3I FOREIGN KEY (label_2_visible) REFERENCES qwat_vl.visible(vl_code_int);
     CREATE INDEX %$4I ON qwat_od.%$1I(label_1_visible);
     CREATE INDEX %$5I ON qwat_od.%$1I(label_2_visible)'
   , _tbl
   , _tbl || '_label_1_visible'
   , _tbl || '_label_2_visible'
   , 'fki_' || _tbl || '_label_1_visible'
   , 'fki_' || _tbl || '_label_2_visible');
END
$func$ LANGUAGE plpgsql;
  • Be sure to use unambiguous, valid parameter names (position being a reserved word was the primary error).

  • I fixed your SQL injection issues with format() (since the simple solution with regclass didn't cover your concatenated names, as commented by @pozs). Detailed explanation:

  • _tbl has to be the unqualified table name ("tbl", not "schema.tbl").

  • Don't quote the language name, it's an identifier: LANGUAGE plpgsql

  • It's cheaper to add multiple columns / sonstraints in a single ALTER TABLE statement.

  • Other assorted simplifications / optimisations.

Community
  • 1
  • 1
Erwin Brandstetter
  • 605,456
  • 145
  • 1,078
  • 1,228
  • 1
    While I generally agree with using `regclass`, this case would generate syntax errors (near constraint names) with special table names, like `"$t"` (it would generate f.ex. `CREATE INDEX fki_"$t"_label_1_visible ...` -- one should use ugly selects from pg catalogs to overcome this, or use `format()` / `quote_ident()` for simplicity – pozs Jan 26 '15 at 15:40
  • @pozs: You are right. I overlooked the concatenated names. Non-standard identifiers would be a problem. `format()` is the way to go. – Erwin Brandstetter Jan 26 '15 at 17:37