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-03 07:13:38
Message-ID: AANLkTi=azWPVodF_KCLi3sEWVhoNSa9iYgyqDR1QuAD0@mail.gmail.com (view raw or flat)
Thread:
Lists: pgsql-hackers
On Thu, Dec 2, 2010 at 20:00, Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr> wrote:
> Please find attached the v8 version of the patch, that fixes the following:

I fixed and cleanup some of codes in it; v9 patch attached. Please check
my modifications, and set the status to "Ready to Committer" if you find
no problems. I think documentation and code comments might need to be
checked by native English speakers.

> Well thinking about it, omitting the length parameter alltogether seems
> like the more natural SQL level API for me, so I've made it happen:

Good idea. I re-added negative lengths checks in pg_read_file functions;
negative length is used internally, but not exposed as SQL functions.

>>   BTW, I think we can call it just "replace"
> The same idea occured to me yesternight when reading through the patch
> to send. It's now done in the way you can see above. The idea is not to
> change the existing behavior at all, so I've not changed the
> non-VARIADIC version of the function.

You added replace(text, text, text, VARIADIC text), but I think
replace(text, VARIADIC text) also works. If we have the short form,
we can use it easily from execute functions with placeholders.

Other changes:
* Added some regression tests.
* Int64GetDatum((int64) fst.st_size) was broken.
* An error checks for "could not read file" didn't work.
* Read file contents into bytea buffer directly to avoid memcpy.
* Fixed bad usages of text and bytea values
  because they are not null-terminated.
* I don't want to expose ArrayType to builtins.h.
  So I call replace_text_variadic() from execute functions.
* pg_execute_sql_file(path, NULL) won't work because it's a STRICT function.
  It returns NULL with no works when at least one of the argument is NULL.

BTW, we have many text from/to cstring conversions in the new codes.
It would be not an item for now, but we would need to improve them
if those functions are heavily used, Especially replace_text_variadic().

>> * We might rename pg_convert_and_execute_sql_file() to
>>   pg_execute_sql_file_with_encoding() or so to have the same prefix.
>
> Well, I think I prefer reading the verbs in the order that things are
> happening in the code, it's actually convert then execute. But again,
> maybe some Native Speaker will have a say here, or maybe your proposed
> name fits better in PostgreSQL. I'd leave it for commiter :)

Agreed. I also prefer pg_read_file_all rather than pg_read_whole_file :P

-- 
Itagaki Takahiro

Attachment: pg_execute_from_file.v9.patch
Description: application/octet-stream (28.5 KB)

In response to

Responses

pgsql-hackers by date

Next:From: Jeroen VermeulenDate: 2010-12-03 08:14:26
Subject: Re: Hypothetical Indexes - PostgreSQL extension - PGCON 2010
Previous:From: Heikki LinnakangasDate: 2010-12-03 06:18:57
Subject: Re: should we set hint bits without dirtying the page?

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