current community

your communities

more stack exchange communities

Stack Exchange
tour help
stack overflow careers
Take the 2-minute tour ×
Stack Overflow is a question and answer site for professional and enthusiast programmers. It's 100% free, no registration required.
up vote 0 down vote favorite

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?

share|improve this question
    
@a_horse_with_no_name it supports (basically it supports any sql expressions postgresql.org/docs/current/static/plpgsql-expressions.html ) -- but of course IF <boolean variable> THEN is way more understandable. –  pozs Jan 26 at 10:08
    
@a_horse_with_no_name: this documentation page suggests that is true is supported –  Andomar Jan 26 at 10:13
3  
position is an SQL reserved word, you should quote it postgresql.org/docs/current/static/sql-keywords-appendix.html –  pozs Jan 26 at 10:13
    
Thanks!!! It was indeed the position which caused troubles. –  Denis Rouzaud Jan 26 at 12:49

1 Answer 1

up vote 2 down vote accepted

@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.

share|improve this answer
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 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 at 17:37
    
Thanks a lot for this detailed revision, and I've learned a few useful things!!! –  Denis Rouzaud Jan 26 at 18:57

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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