pg_execute_from_file review

Joshua Tolley <eggyknap@gmail.com> writes:

I've just looked at pg_execute_from_file[1]. The idea here is to execute all
the SQL commands in a given file. My comments:

Thanks for your review. Please find attached a revised patch where I've
changed the internals of the function so that it's split in two and that
the opr_sanity check passes, per comments from David Wheeler and Tom Lane.

* I'd like to see the docs slightly expanded, specifically to describe
parameter replacement. I wondered for a while if I needed to set of
parameters in any specific way, before reading the code and realizing they
can be whatever I want.

My guess is that you knew that in the CREATE EXTENSION context, it has
been proposed to use the notation @extschema@ as a placeholder, and
you've then been confused. I've refrained from imposing any style with
respect to what the placeholder would look like in the mecanism-patch.

Do we still want to detail in the docs that there's nothing expected
about the placeholder syntax of format?

* Does anyone think it needs representation in the test suite?

Well the patch will get its tests with the arrival of the extension main
patch, where all contribs are installed using it.

* Is it at all bad to include spi.h in genfile.c? IOW should this function
live elsewhere? It seems reasonable to me to do it as written, but I thought
I'd ask.

Well, using spi at this place has been asked by Álvaro and Tom, so my
guess is that's ok :)

* In the snippet below, it seems best just to use palloc0():
query_string = (char *)palloc((fsize+1)*sizeof(char));
memset(query_string, 0, fsize+1);

Edited.

* Shouldn't it include SPI_push() and SPI_pop()?

ENOCLUE

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

On Thu, Nov 25, 2010 at 10:24:51PM +0100, Dimitri Fontaine wrote:

Joshua Tolley <eggyknap@gmail.com> writes:

I've just looked at pg_execute_from_file[1]. The idea here is to execute all
the SQL commands in a given file. My comments:

Thanks for your review. Please find attached a revised patch where I've
changed the internals of the function so that it's split in two and that
the opr_sanity check passes, per comments from David Wheeler and Tom Lane.

I'll take a look ASAP.

* I'd like to see the docs slightly expanded, specifically to describe
parameter replacement. I wondered for a while if I needed to set of
parameters in any specific way, before reading the code and realizing they
can be whatever I want.

My guess is that you knew that in the CREATE EXTENSION context, it has
been proposed to use the notation @extschema@ as a placeholder, and
you've then been confused. I've refrained from imposing any style with
respect to what the placeholder would look like in the mecanism-patch.

Do we still want to detail in the docs that there's nothing expected
about the placeholder syntax of format?

Perhaps such docs will show up with the rest of the EXTENSION work, but I'd
like a brief mention somewhere.

* Does anyone think it needs representation in the test suite?

Well the patch will get its tests with the arrival of the extension main
patch, where all contribs are installed using it.

Works for me.

* Shouldn't it include SPI_push() and SPI_pop()?

ENOCLUE

My guess is "yes", because that was widely hailed as a good idea when I did
PL/LOLCODE. I suspect it would only matter if someone were using
pg_execute_from_file within some other function, which isn't entirely
unlikely.

--
Joshua Tolley / eggyknap
End Point Corporation
http://www.endpoint.com

Joshua Tolley <eggyknap@gmail.com> writes:

* I'd like to see the docs slightly expanded, specifically to describe
parameter replacement. I wondered for a while if I needed to set of
parameters in any specific way, before reading the code and realizing they
can be whatever I want.

My guess is that you knew that in the CREATE EXTENSION context, it has
been proposed to use the notation @extschema@ as a placeholder, and
you've then been confused. I've refrained from imposing any style with
respect to what the placeholder would look like in the mecanism-patch.

Do we still want to detail in the docs that there's nothing expected
about the placeholder syntax of format?

Perhaps such docs will show up with the rest of the EXTENSION work, but I'd
like a brief mention somewhere.

I'm unclear about how to spell out what you'd like to be seen in there,
so I'm not proposing a newer version of the patch.

* Shouldn't it include SPI_push() and SPI_pop()?

ENOCLUE

My guess is "yes", because that was widely hailed as a good idea when I did
PL/LOLCODE. I suspect it would only matter if someone were using
pg_execute_from_file within some other function, which isn't entirely
unlikely.

In fact, per the fine manual, it seems unnecessary doing so when using
SPI_execute(), which is the case here:

http://www.postgresql.org/docs/9.0/interactive/spi-spi-push.html

Note that SPI_execute and related functions automatically do the
equivalent of SPI_push before passing control back to the SQL
execution engine, so it is not necessary for you to worry about this
when using those functions.

For information, we've been talking about the case on IRC and Joshua and
we are agreeing on this conclusion.

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

On Fri, Nov 26, 2010 at 06:24, Dimitri Fontaine <dimitri@2ndquadrant.fr> wrote:

Thanks for your review. Please find attached a revised patch where I've
changed the internals of the function so that it's split in two and that
the opr_sanity check passes, per comments from David Wheeler and Tom Lane.

I have some comments and questions about pg_execute_from_file.v5.patch.

==== Source code ====
* OID=3627 is used by another function. Don't you expect 3927?

* There is a compiler warning:
genfile.c: In function ‘pg_execute_from_file_with_placeholders’:
genfile.c:427: warning: unused variable ‘query_string’

* I'd like to ask native speakers whether "from" is needed in names
of "pg_execute_from_file" and "pg_execute_from_query_string".

==== Design and Implementation ====
* pg_execute_from_file() can execute any files even if they are not
in $PGDATA. OTOH, we restrict pg_read_file() to read such files.
What will be our policy? Note that the contents of file might be
logged or returned to the client on errors.

* Do we have any reasons to have pg_execute_from_file separated from
pg_read_file ? If we allow pg_read_file() to read files in $PGSHARE,
pg_execute_from_file could be replaced with "EXECUTE pg_read_file()".
(Note that pg_execute_from_file is implemented to do so even now.)

* I hope pg_execute_from_file (and pg_read_file) had an option
to specify an character encoding of the file. Especially, SJIS
is still used widely, but it is not a valid server encoding.

--
Itagaki Takahiro

Itagaki Takahiro <itagaki.takahiro@gmail.com> writes:

I have some comments and questions about pg_execute_from_file.v5.patch.

Thanks for reviewing it!

==== Source code ====
* OID=3627 is used by another function. Don't you expect 3927?

Yes indeed. It took me some time to understand what's happening here,
and it seems to be a case of git branches management from me. Here's the
patch as I worked on it (that's much easier to test against the full
branch, that's extension), then as it got cherry-picked into the branch
I use to produce the patches:

http://git.postgresql.org/gitweb?p=postgresql-extension.git;a=commitdiff;h=b406fe35e36e6823e18f7c3157bc330b40b130d4
http://git.postgresql.org/gitweb?p=postgresql-extension.git;a=commitdiff;h=b04eda8f8ee05c3bb5f4d6b693c5169aa7c3b9d1

I missed including a part of the following patch into the
pg_execute_from_file branch:

http://git.postgresql.org/gitweb?p=postgresql-extension.git;a=commitdiff;h=7b3fe8384fb7636130e50f03a338f36495e15030

Sorry about that, will get fixed in v6 — already fixed in the git branch.

* There is a compiler warning:
genfile.c: In function ‘pg_execute_from_file_with_placeholders’:
genfile.c:427: warning: unused variable ‘query_string’

Fixed (in the git branches), sorry about that.

* I'd like to ask native speakers whether "from" is needed in names
of "pg_execute_from_file" and "pg_execute_from_query_string".

Fair enough, will wait for some comments before producing a v6.

==== Design and Implementation ====
* pg_execute_from_file() can execute any files even if they are not
in $PGDATA. OTOH, we restrict pg_read_file() to read such files.
What will be our policy? Note that the contents of file might be
logged or returned to the client on errors.

* Do we have any reasons to have pg_execute_from_file separated from
pg_read_file ? If we allow pg_read_file() to read files in $PGSHARE,
pg_execute_from_file could be replaced with "EXECUTE pg_read_file()".
(Note that pg_execute_from_file is implemented to do so even now.)

pg_execute_from_file started as a very different animal, and I can see
about it using pg_read_file() now, if we also change restrictions. Also,
note that before using SPI an execute failure didn't ouput the whole
input file.

* I hope pg_execute_from_file (and pg_read_file) had an option
to specify an character encoding of the file. Especially, SJIS
is still used widely, but it is not a valid server encoding.

What we agreed on doing in the extension's main patch was to change the
client_encoding before to call into pg_execute_from_file(). So I'd hope
that changing that client side just before to call pg_execute_from_file
would be enough. Is it?

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

On Mon, Nov 29, 2010 at 4:26 AM, Dimitri Fontaine
<dimitri@2ndquadrant.fr> wrote:

* I'd like to ask native speakers whether "from" is needed in names
  of "pg_execute_from_file" and "pg_execute_from_query_string".

Fair enough, will wait for some comments before producing a v6.

Yes, you need the from there.

--
Robert Haas
Native English Speaker

On Mon, Nov 29, 2010 at 10:27 AM, Robert Haas <robertmhaas@gmail.com> wrote:

On Mon, Nov 29, 2010 at 4:26 AM, Dimitri Fontaine
<dimitri@2ndquadrant.fr> wrote:

* I'd like to ask native speakers whether "from" is needed in names
  of "pg_execute_from_file" and "pg_execute_from_query_string".

Fair enough, will wait for some comments before producing a v6.

Yes, you need the from there.

Eh, wait. You definitely need from in pg_execute_from_file(). But
pg_execute_from_query_string() doesn't sound quite right. What does
that function do, anyway?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 11/29/2010 10:30 AM, Robert Haas wrote:

On Mon, Nov 29, 2010 at 10:27 AM, Robert Haas<robertmhaas@gmail.com> wrote:

On Mon, Nov 29, 2010 at 4:26 AM, Dimitri Fontaine
<dimitri@2ndquadrant.fr> wrote:

* I'd like to ask native speakers whether "from" is needed in names
of "pg_execute_from_file" and "pg_execute_from_query_string".

Fair enough, will wait for some comments before producing a v6.

Yes, you need the from there.

Eh, wait. You definitely need from in pg_execute_from_file(). But
pg_execute_from_query_string() doesn't sound quite right. What does
that function do, anyway?

I'm not sure why you need either "from". It just seems like a noise
word. Maybe we could use pg_execute_query_file() and
pg_execute_query_string(), which would be fairly clear and nicely
symmetrical.

cheers

andrew

Andrew Dunstan <andrew@dunslane.net> writes:

I'm not sure why you need either "from". It just seems like a noise
word. Maybe we could use pg_execute_query_file() and
pg_execute_query_string(), which would be fairly clear and nicely
symmetrical.

I'd go with that but need to tell: only pg_execute_query_file() is to be
exposed at the SQL level, the other one is just a backend facility to
share code between the variants with and without placeholders.

If you wonder why have two variants, it's because you can't have
default values (pg_node_tree) in pg_proc.h, and following Tom's
advices.

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

On Mon, Nov 29, 2010 at 10:37 AM, Andrew Dunstan <andrew@dunslane.net> wrote:

On 11/29/2010 10:30 AM, Robert Haas wrote:

On Mon, Nov 29, 2010 at 10:27 AM, Robert Haas<robertmhaas@gmail.com>
 wrote:

On Mon, Nov 29, 2010 at 4:26 AM, Dimitri Fontaine
<dimitri@2ndquadrant.fr>  wrote:

* I'd like to ask native speakers whether "from" is needed in names
  of "pg_execute_from_file" and "pg_execute_from_query_string".

Fair enough, will wait for some comments before producing a v6.

Yes, you need the from there.

Eh, wait.  You definitely need from in pg_execute_from_file().  But
pg_execute_from_query_string() doesn't sound quite right.  What does
that function do, anyway?

I'm not sure why you need either "from". It just seems like a noise word.
Maybe we could use pg_execute_query_file() and pg_execute_query_string(),
which would be fairly clear and nicely symmetrical.

Because you execute queries, not files. Or at least that's how I
think about it.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Andrew Dunstan <andrew@dunslane.net> writes:

I'm not sure why you need either "from". It just seems like a noise
word. Maybe we could use pg_execute_query_file() and
pg_execute_query_string(), which would be fairly clear and nicely
symmetrical.

+1, but I think "query" is also a noise word here.
Why not just "pg_execute_file" and "pg_execute_string"?

regards, tom lane

On 11/29/2010 10:51 AM, Robert Haas wrote:

I'm not sure why you need either "from". It just seems like a noise word.
Maybe we could use pg_execute_query_file() and pg_execute_query_string(),
which would be fairly clear and nicely symmetrical.

Because you execute queries, not files. Or at least that's how I
think about it.

Well, to me "pg_execute_query_file" says "execute the queries in this
file". I'm not sure what else it could sensibly mean. But I think any of
the suggestions will probably work OK.

cheers

andrew

On Mon, Nov 29, 2010 at 11:12:58AM -0500, Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

I'm not sure why you need either "from". It just seems like a noise
word. Maybe we could use pg_execute_query_file() and
pg_execute_query_string(), which would be fairly clear and nicely
symmetrical.

+1, but I think "query" is also a noise word here.
Why not just "pg_execute_file" and "pg_execute_string"?

regards, tom lane

While we're bikeshedding, and since I started the thread that has become this
topic, +1 for Tom's naming.

--
Joshua Tolley / eggyknap
End Point Corporation
http://www.endpoint.com

On 11/29/2010 11:12 AM, Tom Lane wrote:

Andrew Dunstan<andrew@dunslane.net> writes:

I'm not sure why you need either "from". It just seems like a noise
word. Maybe we could use pg_execute_query_file() and
pg_execute_query_string(), which would be fairly clear and nicely
symmetrical.

+1, but I think "query" is also a noise word here.
Why not just "pg_execute_file" and "pg_execute_string"?

Well, I put that in to make it clear that the file/string is expected to
contain SQL and not, say, machine code. But I agree we could possibly do
without it.

cheers

andrew

Andrew Dunstan <andrew@dunslane.net> writes:

On 11/29/2010 11:12 AM, Tom Lane wrote:

+1, but I think "query" is also a noise word here.
Why not just "pg_execute_file" and "pg_execute_string"?

Well, I put that in to make it clear that the file/string is expected to
contain SQL and not, say, machine code. But I agree we could possibly do
without it.

Well, if you want to make that clear, it should be "pg_execute_sql_file"
etc. I still think "query" is pretty vague, if not actually
counterproductive (because it's singular not plural, so someone might
think the file can only contain one query).

regards, tom lane

On Mon, Nov 29, 2010 at 11:12 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

I'm not sure why you need either "from". It just seems like a noise
word. Maybe we could use pg_execute_query_file() and
pg_execute_query_string(), which would be fairly clear and nicely
symmetrical.

+1, but I think "query" is also a noise word here.
Why not just "pg_execute_file" and "pg_execute_string"?

I'd pick pg_execute_from_file() and just plain pg_execute(), myself.

pg_execute_file() could be read to mean you are going to execute the
file itself (i.e. it's a program).

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:

pg_execute_file() could be read to mean you are going to execute the
file itself (i.e. it's a program).

Well, if that's what it suggests to you, I don't see how adding "from"
disambiguates anything. You could be executing machine code "from"
a file, too.

What did you think of "pg_execute_sql_file"?

regards, tom lane

On Mon, Nov 29, 2010 at 11:48 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

pg_execute_file() could be read to mean you are going to execute the
file itself (i.e. it's a program).

Well, if that's what it suggests to you, I don't see how adding "from"
disambiguates anything.  You could be executing machine code "from"
a file, too.

What did you think of "pg_execute_sql_file"?

That, I like.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:

I'd pick pg_execute_from_file() and just plain pg_execute(), myself.

For the record there's only one name exposed at the SQL level. Or do you
want me to expand the patch to actually include a pg_execute() version
of the function, that would execute the query in PG_GETARG_TEXT_P(0)?

On Mon, Nov 29, 2010 at 11:48 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:
What did you think of "pg_execute_sql_file"?

That, I like.

Ok, I call pg_execute_sql_file() the winner and will prepare a new patch
later tonight, now is comute time.

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support