Re: Patch for Statement.getGeneratedKeys()

> The previous discussion does detail the remaining steps needed to get
> something ready to be committed: proper quoting, error checking, test
> cases, ... If you have the time and skill to work on these that would
> be appreciated.
>

Kris (and folks), I did actually find some justification for the time to
work on this. The attached patch should follow on from our last state in
Feb. Essentially the only thing I added was error checking of the
arguments to 'executeUpdate's columnNames. The checking is done in a new
Utils.needsQuoted(String in) (see javadoc) (feel free to rename this
method).

I opted to use the Quoting mechanism I already had in executeUpdate for
now, since the string validation (no 0x00 && no nested quotes) is being
done in needsQuoted (in the same loop that validates quotes and scans
for whitespace).

Let me know in detail what else needs to be implemented in terms of
error checking or methods. I did not add any unit-tests (that will be a
learning curve; seeking volunteers).

Questions:

-is whitespace the sole determinator for needing quoting? And other chars?

-is it fine to leave the string un-quoted if it contains no ws, vs
always quoting it (my feeling is yes).

-is '"' the only legal quoting chars? (I cant remember for having
dabbled with too many non-spec databases)

-my needsQuoted method throws if the identifier contains nested quotes
(foo"bar or "foo"bar"); is there a legal quote-escaping mechanism
similar to apostrophe doubling? eg: how or would one pass foo"bar (I
imagine quotes are never allowed in identifiers but don't have an SQL
spec handy)

Ken

PS - Kris, I recall you said the backslashes in the patch were
troublesome; did you find any fix for your patch tool aside from
translating them to '/'? If not I will translate them from hereto forth.

Version with 4-space instead of tabs..

Does anyone have knowledge of how to implement:

Statement.executeUpdate(String sql, int columnIndexes[])

This would be in the context of using the server's RETURNING clause. For
named-identifiers this is straightforward but I do not know how the
'returning' clause could support numbers... hopefully it can.

Something like?:

... RETURNING [1],[2]

Ken

I am attempting to implement (in a driver)(PG JDBC) support for
specifying which column indexes (that generated keys) to return, so I'm
searching for a way to get the server to return the values of the
columns by their index, not name. By name, it is simply to append the
RETURNING clause and column names to the query:

INSERT... RETURNING foo,bar

Does anyone know how (if) this is possible by index? A standard or
server-specific syntax is fine since this is being implemented in a
server-driver.

Something like?:

INSERT... RETURNING [1],[2] (obviously this will not work)

Would I otherwise need to?:

INSERT... RETURNING *

then extract the user-requested columns? This seems inefficient as it
returns all columns / non-key ones (blobs etc).

While the values of an API that specifies the table's columns by
ordinaility may seem dubious, it is an API that I think should be
implemented anyway.

Thanks,
Ken

Ken Johanson <pg-user(at)kensystem(dot)com> writes:
> While the values of an API that specifies the table's columns by
> ordinaility may seem dubious, it is an API that I think should be
> implemented anyway.

Every few weeks we get a complaint from someone who thinks that it
should be easy to rearrange the logical order of table columns.
If that comes to pass, it would be a seriously bad idea to have
encouraged applications to rely on table column numbers. And given
the number of votes for that (probably in the hundreds by now)
versus the number of votes for this idea (one), I think column
reordering is much more likely to get done ...

regards, tom lane

On Wed, 12 Dec 2007, Tom Lane wrote:

> Every few weeks we get a complaint from someone who thinks that it
> should be easy to rearrange the logical order of table columns.
> If that comes to pass, it would be a seriously bad idea to have
> encouraged applications to rely on table column numbers.

I think the expectation is that:

CREATE TABLE t(a int, b int);
INSERT INTO t(b,a) VALUES (1,2) RETURNING *;

will return 1,2 instead of 2,1 as it does now. In this case the op is
not expecting that the (potentially reorganized) table order is
driving the results, but the order that they've actually specified the
columns in creates the result.

Kris Jurka

Kris Jurka <books(at)ejurka(dot)com> writes:
> I think the expectation is that:

> CREATE TABLE t(a int, b int);
> INSERT INTO t(b,a) VALUES (1,2) RETURNING *;

> will return 1,2 instead of 2,1 as it does now.

Hmm ... I see your point, but on what grounds could one argue that
a "*" targetlist here should return something different from what
"SELECT * FROM t" would return?

I'd say that an app that wants that should write

INSERT INTO t(b,a) VALUES (1,2) RETURNING b,a;

which is surely not that hard if you've got the code to produce
the "(b,a)" part.

In any case it's not clear this is the same thing Ken is complaining
about ...

regards, tom lane

Tom Lane wrote:
> Kris Jurka <books(at)ejurka(dot)com> writes:
>> I think the expectation is that:
>
>> CREATE TABLE t(a int, b int);
>> INSERT INTO t(b,a) VALUES (1,2) RETURNING *;
>
>> will return 1,2 instead of 2,1 as it does now.
>
> Hmm ... I see your point, but on what grounds could one argue that
> a "*" targetlist here should return something different from what
> "SELECT * FROM t" would return?
>
> I'd say that an app that wants that should write
>
> INSERT INTO t(b,a) VALUES (1,2) RETURNING b,a;
>
> which is surely not that hard if you've got the code to produce
> the "(b,a)" part.
>
> In any case it's not clear this is the same thing Ken is complaining
> about ...
>

I am only seeking to have the columns returned in the order they appear
naturally. JDBC says "This array contains the indexes of the columns in
the target table that contain the auto-generated keys that should be
made available."

For the record I was not "complaining", only citing in advance the fact
that while some consider selecting the keys by index to be dubious, it
nonetheless must be done because an API requires it. Casting my question
into a complaint is another topic.

Kris Jurka wrote:
>
> I think the expectation is that:
>
> CREATE TABLE t(a int, b int);
> INSERT INTO t(b,a) VALUES (1,2) RETURNING *;
>
> will return 1,2 instead of 2,1 as it does now. In this case the op is
> not expecting that the (potentially reorganized) table order is driving
> the results, but the order that they've actually specified the columns
> in creates the result.
>

Kris, do you have pointers to a spec that says the named-columns should
specify the index, or that it should instead be the order in the table?
My interpretation from the JDBC spec was that the latter is true, I may
be wrong...

In the case where it is table-order, then I presume in PG that the
"natural" order of the columns (even if reordering is allowed at a alter
date) is specified by data in one of the pg_* tables (pg_class,
pg_index, etc). Does anyone know if this is true/false?

If true, my next idea would be to derive the column name using a
subquery in the returning clause. But it sounds like this may have
potential security contraints (will any INSERT query always have read
access to the PG tables?). And no guarantee of the order matching in the
long term.

Is there a more elegant approach, like?:

INSERT... RETURNING (PG_LIST_KEYS(tblname))

I looked but did not find such a utility. It seems that such function
would be best implemented in the server instead of in a driver (eg.
having hardcoded subquery to the schema).

Ken

Kris, do you what token parsers utils exists in the current JDBC
package? E.g the most tried and true way to get the schema and table
name from:

INSERT INTO foo (col1, col2..) VALUES ..
INSERT INTO foo VALUES ..
INSERT INTO "foo" VALUES ..
INSERT INTO mydb."foo" VALUES ..
etc.

Thanks,
Ken

On Wed, 12 Dec 2007, Ken Johanson wrote:

> Kris, do you have pointers to a spec that says the named-columns should
> specify the index, or that it should instead be the order in the table? My
> interpretation from the JDBC spec was that the latter is true, I may be
> wrong...

No, I was actually misremembering what the JDBC spec said, although I
think it's an interesting case to consider regardless of any specs.

> In the case where it is table-order, then I presume in PG that the "natural"
> order of the columns (even if reordering is allowed at a alter date) is
> specified by data in one of the pg_* tables (pg_class, pg_index, etc). Does
> anyone know if this is true/false?

pg_attribute.attnum stores column order at the moment. If/when
reordering is allowed, there will be another column indicating the
logical order of the colums.

> If true, my next idea would be to derive the column name using a subquery in
> the returning clause. But it sounds like this may have potential security
> contraints (will any INSERT query always have read access to the PG tables?).
> And no guarantee of the order matching in the long term.

There is no requirement that insert permission on a user table implies
read access to pg_catalog. Still many clients will break if they can't
read pg_catalog. For example, all of the JDBC driver's MetaData results
need to query pg tables, updatable ResultSets need to query pg tables to
know what the primary key is and so on. So if this functionality required
access to pg_catalog that would neither be unprecedented nor unreasonable.

> Is there a more elegant approach, like?:
>
> INSERT... RETURNING (PG_LIST_KEYS(tblname))
>

You can't dynamically derive the returning clause for the same reason you
can't say "INSERT INTO (SELECT myfunc()) VALUES (...)", using myfunc to
determine the table name at runtime. The planner needs to know all the
tables/columns/other database parts up front before executing anything.

Kris Jurka

On Wed, 12 Dec 2007, Ken Johanson wrote:

> Kris, do you what token parsers utils exists in the current JDBC
> package? E.g the most tried and true way to get the schema and table
> name from:
>
> INSERT INTO foo (col1, col2..) VALUES ..

Most of the parsing code in the driver is focused on finding placeholders
and escape sequences and doesn't care what the query is actually doing.
Deriving the base tables of a query happens in only one place, updatable
resultset support. See
org.postgresql.jdbc2.AbstractJdbc2ResultSet#parseQuery. That said, the
current implementation is terrible and is fooled by many queries. It just
looks for the first " FROM " and takes anything after that as the table
the select is based on. Clearly this doesn't work for SELECT col AS from
FROM tab; or SELECT /* FROM */ * FROM tab; or SELECT (SELECT col FROM tab)
FROM tab2; and many other ways. So I doubt modelling new code on it is a
good idea.

Kris Jurka

>
>> If true, my next idea would be to derive the column name using a
>> subquery in the returning clause. But it sounds like this may have
>> potential security contraints (will any INSERT query always have read
>> access to the PG tables?). And no guarantee of the order matching in
>> the long term.
>
> There is no requirement that insert permission on a user table implies
> read access to pg_catalog. Still many clients will break if they can't
> read pg_catalog. For example, all of the JDBC driver's MetaData results
> need to query pg tables, updatable ResultSets need to query pg tables to
> know what the primary key is and so on. So if this functionality
> required access to pg_catalog that would neither be unprecedented nor
> unreasonable.
>

So it sounds like this may be the best approach, do you agree? I'll try
and find the cycles to code this up although the limited value of
getGeneratedKeys by index makes me think my time would be better spent
elsewhere on the JDBC driver. For now at least. If you can respond to my
earlier query (5 Dec) about what robustness improvements are needed,
I'll start there..

Thanks,
Ken

On Wed, 5 Dec 2007, Ken Johanson wrote:

> I opted to use the Quoting mechanism I already had in executeUpdate for now,
> since the string validation (no 0x00 && no nested quotes) is being done in
> needsQuoted (in the same loop that validates quotes and scans for
> whitespace).
>
> -is whitespace the sole determinator for needing quoting? And other chars?

Any keywords would need quoting: If you had a column named "user" it must
be quoted.

jurka=# create temp table zz(a int, "user" text);
CREATE TABLE

jurka=# insert into zz values(1,'a') returning a, user, "user";
a | current_user | user
---+--------------+------
1 | jurka | a

> -is it fine to leave the string un-quoted if it contains no ws, vs always
> quoting it (my feeling is yes).

I was thinking about this some more and I think we should quote everything
regardless of whether it needs it or not. This forces the caller to
provide the column in the correct case because it won't be folded any
more, but that's something we're already doing in DatabaseMetaData. If we
don't do this there will be no way for the user to indicate that he has
case-sensitive column names. (Unless of course we implemented
getGeneratedKeys with column names similar to how we might implemented it
for interger column indexes. If we used RETURNING * and only did the
extraction once it got back to the driver, then we have some more
flexibility in handling names.)

> -is '"' the only legal quoting chars? (I cant remember for having dabbled
> with too many non-spec databases)

Yes.

> -my needsQuoted method throws if the identifier contains nested quotes
> (foo"bar or "foo"bar"); is there a legal quote-escaping mechanism similar to
> apostrophe doubling? eg: how or would one pass foo"bar (I imagine quotes are
> never allowed in identifiers but don't have an SQL spec handy)

Nested quotes are legal and escaped just like apostrophe doubling:

create table "abc""def" ( """" int);

> PS - Kris, I recall you said the backslashes in the patch were troublesome;
> did you find any fix for your patch tool aside from translating them to '/'?
> If not I will translate them from hereto forth.
>

I haven't looked at it since then. Let's get another draft or two and
I'll see what needs to be done.

Kris Jurka

Kris and all,

Here is the query I will call to the get the name of columns by ordinal
position. Do you see any compatibility drivers will older server
versions, or other issues?

SELECT column_name
FROM information_schema.columns
WHERE table_catalog=? AND table_schema=? AND table_name=?
ORDER BY ordinal_position

Ken

Kris Jurka wrote:

>
> Any keywords would need quoting: If you had a column named "user" it
> must be quoted.
>

Enough said. I will quote all IDs and provide a diff tonight hopefully.

On Thu, 13 Dec 2007, Ken Johanson wrote:

> Here is the query I will call to the get the name of columns by ordinal
> position. Do you see any compatibility drivers will older server versions, or
> other issues?
>
> SELECT column_name
> FROM information_schema.columns
> WHERE table_catalog=? AND table_schema=? AND table_name=?
> ORDER BY ordinal_position
>

Using pg_catalog tables is better than using information_schema because of
the way permissions work. For information_schema you must be the table
owner, while people who only have permissions to access a table will most
likely be able to read pg_catalog.

Kris Jurka

Kris Jurka wrote:

>
> Using pg_catalog tables is better than using information_schema because
> of the way permissions work. For information_schema you must be the
> table owner, while people who only have permissions to access a table
> will most likely be able to read pg_catalog.
>

Do you have an equivalent query/join handy that will get the catalog and
schema and table and column names frm the pg tables?

SELECT column_name
FROM information_schema.columns
WHERE table_catalog=? AND table_schema=? AND table_name=?
ORDER BY ordinal_position

Kris, please try to apply the attached and let me know what errors if
any you get.

All ids are now quoted in: executeUpdate(String sql, String
columnIndexes[]), and: int executeUpdate(String sql, int
columnIndexes[]) is implemented, however it currently will work ONLY if
the fully qualified table is set in the insert:

INSERT INTO foocatalog.fooschema.tbl .....(quoted or not)

It will support normalizing the not-supplied catalog and schema names --
after I find out how to extract these from the Connection (hopefully
this would not require an additional round trip). Any suggestions on this?

Ken

Kris, were you able to apply the last patch I sent? Let me know what
you'd like. I would like to proceed so I can close this project out.

On Tue, 18 Dec 2007, Ken Johanson wrote:

> Kris, were you able to apply the last patch I sent? Let me know what
> you'd like. I would like to proceed so I can close this project out.
>

Sorry, I have not had time to look at it and probably won't be able to
until next year. I've got a vacation coming up and people who pay me that
want stuff done before I leave.

Kris Jurka

> On Tue, 18 Dec 2007, Ken Johanson wrote:
>
>> Kris, were you able to apply the last patch I sent? Let me know what
>> you'd like. I would like to proceed so I can close this project out.
>>
>
> Sorry, I have not had time to look at it and probably won't be able to
> until next year. I've got a vacation coming up and people who pay me
> that want stuff done before I leave.
>

Just a friendly reminder. Whenever you can try the patch let me know.
k

On Fri, 14 Dec 2007, Ken Johanson wrote:

> Kris, please try to apply the attached and let me know what errors if
> any you get.

This patch is completely busted. It uses backslashes instead of forward
slashes, which is relatively easily fixed, but it also has wrong line
numbers. Consider this section of the patch:

***************
*** 159,172 ****
*/
public int executeUpdate(String sql, int columnIndexes[]) throws
SQLException
{
! if (columnIndexes.length == 0)
return executeUpdate(sql);
!
! throw new PSQLException(GT.tr("Returning autogenerated keys is
not supported."), PSQLState.NOT_IMPLEMENTED);
}

/**
--- 166,206 ----

Here it claims to have lines 159 to 172, but it only has 10 lines of text.
Perhaps you need a Netbeans upgrade or you need to use some other
CVS client.

Reading through the patch I have the following comments:

1) Why does executeUpdateGetResults have support for isFunction? Doesn't
that require preparedQuery != null? Even if not, shouldn't the
replaceProcessing be before the isFunction check?

2) Shouldn't the result for a generated key result be stored in some
place more specific? Right now can't you issue executeQuery() and then
call getGeneratedKeys()?

3) Generated keys should work for more than just insert, at least in
postgres' design. RETURNING works for all of INSERT/UPDATE/DELETE.

4) As discussed previously on -general the code in executeUpdate(String,
int[]) shouldn't be using information_schema because that has additional
permission requirements. Also it looks like it's got sql injection
problems.

5) There's no need to check connection instanceof AbstractJdbc2Connection
in executeUpdate(String, String[]). That will always be true.

6) There's no need to split this up for translation purposes, just
make it one string:

throw new PSQLException(GT.tr("Server version does not support
returning generated keys.")+" (< "+"8.2"+")", PSQLState.NOT_IMPLEMENTED);

7) executeUpdate(String, String[]) does not correctly escape the
columnNames provided if the values have embedded quotes.

8) Utils.needsQuoted is unused and should be removed.

9) Utils.getInsertIds doesn't look right. Looks like it will return
"into" for something like "insert into xxx (...)". It doesn't look
like it will work for names with quotes in them like "x""y". Also
the requirement that a query uses a completely qualified name
database.schema.table is quite onerous. Additionally the fact that this
requirement is not checked will result in many
ArrayIndexOutOfBoundsExceptions.

10) What's the purpose of Utils.position? How is this better than
String.indexOf with lowercase strings?

Kris Jurka

Kris,
Incomplete / short response below:
>
>
> Here it claims to have lines 159 to 172, but it only has 10 lines of
> text. Perhaps you need a Netbeans upgrade or you need to use some other
> CVS client.

I am upgrading my NB now and we'll see how the next one comes out..

>
> Reading through the patch I have the following comments:
>
> 1) Why does executeUpdateGetResults have support for isFunction?
> Doesn't that require preparedQuery != null? Even if not, shouldn't the
> replaceProcessing be before the isFunction check?

I'm going to let you suggest code for this method, as I don't even
recall coding it (a long time ago perhaps and lazy copy-paste). I do not
know the driver innards enough to answer this (I am a relative novice
even at the formal JDBC API level).

>
> 2) Shouldn't the result for a generated key result be stored in some
> place more specific? Right now can't you issue executeQuery() and then
> call getGeneratedKeys()?

Again I am not familiar with all the use cases but presume you mean,
allowing for calling a query (non-DML?) and then expect the keys to be
available?..

>
> 3) Generated keys should work for more than just insert, at least in
> postgres' design. RETURNING works for all of INSERT/UPDATE/DELETE.
>

Will add these unless you suggest code first.

> 4) As discussed previously on -general the code in executeUpdate(String,
> int[]) shouldn't be using information_schema because that has additional
> permission requirements. Also it looks like it's got sql injection
> problems.

I am waiting on this and #9.b; for an answer to a prior question about:
is it is possible to determine the connection's db and schema names to
normalize those array elements (in the case where just the
[schema]tablename is provided). Is it possible without a round trip to
know what these should be?

>
> 5) There's no need to check connection instanceof
> AbstractJdbc2Connection in executeUpdate(String, String[]). That will
> always be true.

OK

>
> 6) There's no need to split this up for translation purposes, just make
> it one string:
>
> throw new PSQLException(GT.tr("Server version does not support
> returning generated keys.")+" (< "+"8.2"+")", PSQLState.NOT_IMPLEMENTED);
>

Unless the code will not work I will elect to keep support for
translation should someone want to enter these.

> 7) executeUpdate(String, String[]) does not correctly escape the
> columnNames provided if the values have embedded quotes.
>

Thank you, I will apply appendEscapedIdentifier().

> 8) Utils.needsQuoted is unused and should be removed.

OK

>
> 9) Utils.getInsertIds doesn't look right. Looks like it will return
> "into" for something like "insert into xxx (...)". It doesn't
> look like it will work for names with quotes in them like "x""y".

It requires a context-sensitive (int)start argument, to position the
search start onto the first keyword, i.e INTO for the case of INSERTs. I
tested quoted quote-less Ids. As stated earlier it is incomplete pending
a way to pass-in the current DB & schema names.

9.b):
Also
> the requirement that a query uses a completely qualified name
> database.schema.table is quite onerous. Additionally the fact that this
> requirement is not checked will result in many
> ArrayIndexOutOfBoundsExceptions.

(see above). I believe that a fully qualified value may be needed in the
case where the table is fully qualified but (for some reason) not in
the same context as the RETURNING... this can happen, yes? Or does
RETURNING always refer to the acted-on table?

>
> 10) What's the purpose of Utils.position? How is this better than
> String.indexOf with lowercase strings?

It save a potential buffer allocation should String.class find a
case-fold is necessary (esp in large query).

query.toLowerCase().indexOf(s);

Utils.position is not strictly needed and I will remove it if you prefer.

Ken

Kris Jurka wrote:
>
> 9) Utils.getInsertIds
....
> the requirement that a query uses a completely qualified name
> database.schema.table is quite onerous.
>

What are your thoughts on this? Should it be possible/safe going forward
to not supply the fully qualified table in the case of:

Statement.executeUpdate(String sql, int columnIndexes[])

If you can recommend a simpler strategy I will try it. If we can avoid
the round trip also that would be great.

Ken

On Mon, 7 Jan 2008, Ken Johanson wrote:

>> 2) Shouldn't the result for a generated key result be stored in some place
>> more specific? Right now can't you issue executeQuery() and then call
>> getGeneratedKeys()?
>
> Again I am not familiar with all the use cases but presume you mean,
> allowing for calling a query (non-DML?) and then expect the keys to be
> available?..

What I'm saying is that since the "result" variable is set for every
ResultSet, someone can do executeQuery("SELECT ...") and then if they
call getGeneratedKeys it will return a reference to that ResultSet.
getGenereatedKeys should fail if it was not immediately preceded by a
call that created a ResultSet of generated keys.

>> 4) As discussed previously on -general the code in executeUpdate(String,
>> int[]) shouldn't be using information_schema because that has additional
>> permission requirements. Also it looks like it's got sql injection
>> problems.
>
> I am waiting on this and #9.b; for an answer to a prior question about:
> is it is possible to determine the connection's db and schema names to
> normalize those array elements (in the case where just the
> [schema]tablename is provided). Is it possible without a round trip to
> know what these should be?

You can tell the connection's database via Connection.getCatalog, but
there is no such thing as a connection's schema. Each connection has a
search_path that specifies the order it looks through schemas to find a
table. So you can't tell what schema a table is in without looking
through the list.

>> 6) There's no need to split this up for translation purposes, just make it
>> one string:
>>
>> throw new PSQLException(GT.tr("Server version does not support
>> returning generated keys.")+" (< "+"8.2"+")", PSQLState.NOT_IMPLEMENTED);
>>
>
> Unless the code will not work I will elect to keep support for translation
> should someone want to enter these.

I'm not saying it shouldn't be translatable I'm just saying put the "
< 8.2" part into the message string.

>> 10) What's the purpose of Utils.position? How is this better than
>> String.indexOf with lowercase strings?
>
> It save a potential buffer allocation should String.class find a case-fold is
> necessary (esp in large query).
>
> query.toLowerCase().indexOf(s);
>
> Utils.position is not strictly needed and I will remove it if you prefer.
>

Let's take it out.

Kris Jurka

On Wed, 9 Jan 2008, Ken Johanson wrote:

>> the requirement that a query uses a completely qualified name
>> database.schema.table is quite onerous.
>>
>
> What are your thoughts on this? Should it be possible/safe going forward to
> not supply the fully qualified table in the case of:
>
> Statement.executeUpdate(String sql, int columnIndexes[])
>
> If you can recommend a simpler strategy I will try it. If we can avoid the
> round trip also that would be great.
>

Writing "RETURNING *" will return more data than you need, but it has the
plus of not requiring you to know anything about the underlying table
columns. Once the full result is returned you would need to strip out
only the parts specified in columnIndexes and either create a new
ResultSet or a wrapper around the returned ResultSet.

Kris Jurka

>> is it is possible to determine the connection's db and schema
>> names to normalize those array elements (in the case where just the
>> [schema]tablename is provided). Is it possible without a round trip to
>> know what these should be?
>
> You can tell the connection's database via Connection.getCatalog, but
> there is no such thing as a connection's schema. Each connection has a
> search_path that specifies the order it looks through schemas to find a
> table. So you can't tell what schema a table is in without looking
> through the list.
>

That makes sense to me now, thanks. In any case do you agree that we
still need to parse the fully qualified table, in case of input like:

insert into postgres.public.test ...

I don think my earlier question about getting the current/default
catalog name is relevant since the query can specify other ones. True?

>> What are your thoughts on this? Should it be possible/safe going
>> forward to not supply the fully qualified table in the case of:
>>
>> Statement.executeUpdate(String sql, int columnIndexes[])
>>
>> If you can recommend a simpler strategy I will try it. If we can avoid
>> the round trip also that would be great.
>>
>
> Writing "RETURNING *" will return more data than you need, but it has
> the plus of not requiring you to know anything about the underlying
> table columns. Once the full result is returned you would need to strip
> out only the parts specified in columnIndexes and either create a new
> ResultSet or a wrapper around the returned ResultSet.
>

"RETURNING *" is indeed simpler although based on on real word
experience on slow/congested links or hi transaction volume, and use
cases with large inserts (multi-row, many columns, or LOBs) (
memory/scalability issues), the round trip seems to preferable. I'd
rather muddle though getting that to work (I did already and code to
follow) instead of risking more serious performance issues, if you don't
object.

Ken

On Mon, 14 Jan 2008, Ken Johanson wrote:

> That makes sense to me now, thanks. In any case do you agree that we still
> need to parse the fully qualified table, in case of input like:
>
> insert into postgres.public.test ...

That depends what your approach is for non-qualified tables as it would be
odd to do it differently for the two cases. (Just got your other email).
Since you don't like "RETURNING *", you will need to be able to parse a
fully qualified name, but you also must be able to parse and then qualify
a non-fully qualified name.

> I don think my earlier question about getting the current/default catalog
> name is relevant since the query can specify other ones. True?
>

Not really, you can only specify the current database. If you try
it with something else you get:

jurka=# select * from jurka.schema.tab;
ERROR: schema "schema" does not exist

jurka=# select * from otherdb.schema.tab;
ERROR: cross-database references are not implemented:
"otherdb.schema.tab"

Kris Jurka

>> I don think my earlier question about getting the current/default
>> catalog name is relevant since the query can specify other ones. True?
>>
>
> Not really, you can only specify the current database. If you try it
> with something else you get:
>
> jurka=# select * from jurka.schema.tab;
> ERROR: schema "schema" does not exist
>
> jurka=# select * from otherdb.schema.tab;
> ERROR: cross-database references are not implemented: "otherdb.schema.tab"
>

Hmm. I do know that I can select the fully qualified name (select * from
postgres.public.test), so I presume your first example is the case where
"schema" does not exist.

For int executeUpdate(String sql, int columnIndexes[]) I'll still need
get the fully qual'd name to filter the information_schema (or pg_
tables). Since we can get the table name and catalog, that leaves
schema. Do you know how we can get the current schema name? I presumed
it might appear in SHOW ALL, but not so.

Ken

Ken Johanson wrote:
> Do you know how we can get the current schema name? I presumed
> it might appear in SHOW ALL, but not so.
>

Answer: select current_schema() :-)

On Tue, 15 Jan 2008, Ken Johanson wrote:

> Ken Johanson wrote:
>> Do you know how we can get the current schema name? I presumed it might
>> appear in SHOW ALL, but not so.
>>
>
> Answer: select current_schema() :-)
>

False, as I explained earlier there is no current schema. Have
a read of this:

http://www.postgresql.org/docs/8.2/static/ddl-schemas.html#DDL-SCHEMAS-PATH

You need to look through the schemas in the search_path in order and see
which one a table with the given name appears in first.

Kris Jurka

Kris Jurka <books(at)ejurka(dot)com> writes:
> You need to look through the schemas in the search_path in order and see
> which one a table with the given name appears in first.

I've lost track of the context in which this needs to be done, but in
some cases a cast to or from regclass offers a painless way to
disambiguate table names. Just a suggestion ...

regards, tom lane

Tom Lane wrote:
> Kris Jurka <books(at)ejurka(dot)com> writes:
>> You need to look through the schemas in the search_path in order and see
>> which one a table with the given name appears in first.
>
> I've lost track of the context in which this needs to be done, but in
> some cases a cast to or from regclass offers a painless way to
> disambiguate table names. Just a suggestion ...
>

Tom, can you offer an example of this and how the overall goal might be
achieved? Kris, please jump in where I'm missing anything:

#Overview:
We need to implement:
Statement.executeUpdate(String sql, int columnIndexes[])

Current strategy is to find the natural column order (ordinal positions
for columnIndexes[]) and extract those names, passing them through to:
Statement.executeUpdate(String sql, String columnIndexes[])

To get the column names, I need to look in [the pg_* table equiv to
information_schema] tables, and of course this means knowing which table
is being referenced for modification. We are already parsing the table
name (fully or partially qualified) from the DML; now we need to search
[information_schema], finding the matching catalog, schema, and table,
and searching schema in the order of the schema search-path.

#History
Most interesting probably is that Kris mentioned it would work to just
do a INSERT.. RETURNING * to get the keys, however I'm electing to try
the extra-mile / "hard way" to save returning LOBs or entire multi-row
inserts. Ideally I's like to do everything in one extra query to
[information_schema] or better yet in RETURNING.

#Now
I'm a bit perplexed as to how I could get the current-ref'd schema using
one query. I think it might involve passing a subselect of "SHOW
search_path" as the arg to the [information_schema] query, but using
that var as a list and filling the $user var is not familiar ground...

#Questions:

-would the regclass-cast technique (I have no experience with it) work
directly in the RETURNING or need to be in the [information_schema]
query? Can you point me to examples?

-would it be feasible to modify RETURNING in new server versions to
accept indexes as args? That would obviate this whole discussion.

Thanks,
Ken

On Wed, 16 Jan 2008, Ken Johanson wrote:

>> I've lost track of the context in which this needs to be done, but in
>> some cases a cast to or from regclass offers a painless way to
>> disambiguate table names. Just a suggestion ...
>
> Tom, can you offer an example of this and how the overall goal might be
> achieved? Kris, please jump in where I'm missing anything:

Regclass is actually exactly what you need. This let's us skip all kinds
of parsing, deducing, ...

SELECT 'database.schema.table'::regclass::oid;
SELECT 'schema.table'::regclass::oid;
SELECT 'table'::regclass::oid;
SELECT '"database".schema."table"'::regclass::oid;

will all return the same thing, the oid for the qualified table, or if
unqualified, the first matching table on the search path. The oid will be
pg_class.oid which can then easily be used to lookup the columns in
pg_attribute as people have explained on -general.

> #Questions:
>
> -would the regclass-cast technique (I have no experience with it) work
> directly in the RETURNING or need to be in the [information_schema] query?
> Can you point me to examples?

Needs to be in a separate query.

> -would it be feasible to modify RETURNING in new server versions to accept
> indexes as args? That would obviate this whole discussion.
>

Not really, RETURNING is an arbitrary SELECT list, so you can say things
like RETURNING 1, 2+columnA, f(columnB). You could potentially add some
kind of keyword like RETURNING INDEXES 1,2,7, but I doubt the server
people have a great desire to support something this braindead for just
one client API to use.

Kris Jurka

Ken Johanson <pg-user(at)kensystem(dot)com> writes:
> Tom Lane wrote:
>> I've lost track of the context in which this needs to be done, but in
>> some cases a cast to or from regclass offers a painless way to
>> disambiguate table names. Just a suggestion ...

> Tom, can you offer an example of this and how the overall goal might be
> achieved?

Well, most of the point is that regclass can substitute for an explicit
understanding of search_path disambiguation. If you see "s.t" in the
query then it's clear that this is table t in schema s, but if you see
just "t" then you aren't so sure which schema it is in. Resolving that
in a pure-SQL query is theoretically possible but it seems mighty ugly.

> To get the column names, I need to look in [the pg_* table equiv to
> information_schema] tables, and of course this means knowing which table
> is being referenced for modification.

Right. I suggest using regclass to obtain the OID of the referenced
table, which then allows direct lookup in pg_attribute and other
relevant catalogs. Something along the line of

select attname from pg_attribute where attrelid = 't'::regclass

which also works for

select attname from pg_attribute where attrelid = 's.t'::regclass

This is oversimplified because it doesn't consider any quoting issues
for funny characters in table names, and the query itself needs some
refinements like checking for attisdropped, but hopefully the point
is clear.

> -would it be feasible to modify RETURNING in new server versions to
> accept indexes as args? That would obviate this whole discussion.

I'm not clear what you're hoping for there. It seems to me that any way
you slice it, you'll need to know which columns of the result are what.
"RETURNING *" is inadequate for that reason, but so would be "RETURNING
some-index-columns-but-I-aint-saying-which". ISTM you've got to parse
things out enough to understand which columns you want and why; once
you know that, asking for them by name doesn't seem like much trouble.

regards, tom lane

Tom Lane wrote:
> Right. I suggest using regclass to obtain the OID of the referenced
> table, which then allows direct lookup in pg_attribute and other
> relevant catalogs. Something along the line of
>
> select attname from pg_attribute where attrelid = 't'::regclass
>

Here is what I have so far (not sure of my conditionals yet or if I need
any joins for them):

select attname from pg_attribute
where attrelid = 'postgres.public.test'::regclass
and attstattarget=-1
and attisdropped='f'
order by attnum asc

Very simple and elegant, you guys rule.

Couple things I want to verify - will this regclass method have any
cases where security would make it less reliable than say, using pg_*?
Also compatibility issues?: are there any server or session/driver modes
that would somehow prevent this from working?

ken

BTW: How about next way:
extract table name as it is
parse + describe "select * from tableName".
optionally close unnamed statement (Protocol says it will still be
closed by next unnamed statement).
Read column names from there.
Call insert with returning column names.
It still has round-trip (or two, dunno if parse + describe needs two
round-trips), but it will take any insert statement.

P.S. Sorry if message will come twice - used wrong From: for the first one.

Ken Johanson <pg-user(at)kensystem(dot)com> writes:
> Here is what I have so far (not sure of my conditionals yet or if I need
> any joins for them):

> select attname from pg_attribute
> where attrelid = 'postgres.public.test'::regclass
> and attstattarget=-1
> and attisdropped='f'
> order by attnum asc

I'd do "where attrelid = ... and attnum > 0 and not attisdropped".
Not sure what you think the attstattarget condition would accompllish,
but it doesn't seem like anything that client-side code ought to be
touching.

regards, tom lane

Kris, all: please see the changes in AbstractJdbc3Statement, the
regclass technique suggested by Tom is implemented and working. However
I have not made any attempt to manage the separate generated-keys
resultset nor state (RETURN_GENERATED_KEYS).

Please consider making these changes yourself, or tell me what needs to
be done (again I am not proficient with the spec or state mgmt for the
calls). I think your deeper knowledge of the driver and spec will
produce better quality code. I am losing some cycles starting next week
so won't be able to work on this for several weeks or more.

Thanks,
Ken