Skip site navigation (1) Skip section navigation (2)

Re: pg_execute_from_file review

From: Itagaki Takahiro <itagaki(dot)takahiro(at)gmail(dot)com>
To: Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr>
Cc: Joshua Tolley <eggyknap(at)gmail(dot)com>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: pg_execute_from_file review
Date: 2010-12-02 03:47:23
Message-ID: AANLkTikMHdcwCruCEeJ0V+OB-F2444SwaT4xJg1uDfQQ@mail.gmail.com (view raw or flat)
Thread:
Lists: pgsql-hackers
On Thu, Dec 2, 2010 at 07:00, Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr> wrote:
> Here's the result:
>
> dim=# \df pg_exe*|replace_*|*binary*
>                                     List of functions
>    Schema   |         Name          | Result data type |    Argument data types    |  Type
> ------------+-----------------------+------------------+---------------------------+--------
>  pg_catalog | pg_execute_sql_file   | void             | text                      | normal
>  pg_catalog | pg_execute_sql_file   | void             | text, name                | normal
>  pg_catalog | pg_execute_sql_file   | void             | text, name, VARIADIC text | normal
>  pg_catalog | pg_execute_sql_string | void             | text                      | normal
>  pg_catalog | pg_execute_sql_string | void             | text, VARIADIC text       | normal
>  pg_catalog | pg_read_binary_file   | bytea            | text, bigint, bigint      | normal
>  pg_catalog | replace_placeholders  | text             | text, VARIADIC text       | normal
> (7 rows)

Thanks, good job!

* pg_read_binary_file_internal() should return not only the contents
  as char * but also the length, because the file might contain 0x00.
  In addition, null-terminations for the contents buffer is useless.

* The 1st argument of pg_convert must be bytea rather than cstring in
  pg_convert_and_execute_sql_file(). I think you can fix both the bug
  and the above one if pg_read_binary_file_internal() returns bytea.

* pg_read_file() has stronger restrictions than pg_read_binary_file().
  (absolute path not allowed) and -1 length is not supported.
  We could fix pg_read_file() to behave as like as pg_read_binary_file().

* (It was my suggestion, but) Is it reasonable to use -1 length to read
  the while file? It might fit with C, but NULL might be better for SQL.

* The doc says pg_execute_sql_string() is restricted for superusers,
  but is not restricted actually. I think we don't have to.

* In docs, the example of replace_placeholders() is
  replace('abcdefabcdef', 'cd', 'XX', 'ef', 'YY').
  ~~~~~~~
  BTW, I think we can call it just "replace" because parser can handle
  them correctly even if we have both replace(text, text, text) and
  replace(text, VARIADIC text[]). We will need only one doc for them.
  Note that if we call replace() with 3 args, the non-VARIADIC version
  is called. So, there is no performance penalty.

* We might rename pg_convert_and_execute_sql_file() to
  pg_execute_sql_file_with_encoding() or so to have the same prefix.

-- 
Itagaki Takahiro

In response to

Responses

pgsql-hackers by date

Next:From: Florian PflugDate: 2010-12-02 03:53:59
Subject: Re: improving foreign key locks
Previous:From: Tom LaneDate: 2010-12-02 03:16:11
Subject: Re: build problem

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group