V3 protocol + DECLARE problems

(cc: -hackers as I think this has been raised there before)

It's going to be fun using anything more than very basic cursors via the
V3 protocol in the JDBC driver. DECLARE does not work with parameters
passed via a Parse/Bind combination -- which is how we currently always
pass parameters when talking V3. I'm reluctant to have two
implementations of the parameter logic for V3 (one that does direct
substitution, one that uses Bind) since that's extra unnecessary code
and a recipe for inconsistent behaviour.

Logs follow; basically this is issuing a Parse/Bind/Execute for a
parameterized DECLARE, which blows up with "no value found for parameter
1" despite there definitely being one there. (also, that error appears
on Execute, not Parse/Bind).

Any chance of getting this fixed for 7.5? Alternatively, if we can get
WITH HOLD / SCROLL behaviour in portals created by Execute (probably
means a protocol change) that works too. I don't have a runnable 7.5 on
hand to test against so it's possible this has already been fixed.

-O

> Trying to establish a protocol version 3 connection to localhost:5432
> FE=> StartupPacket(user=oliver, database=test, client_encoding=UNICODE, DateStyle=ISO)
> <=BE AuthenticationOk
> <=BE ParameterStatus(client_encoding = UNICODE)
> <=BE ParameterStatus(DateStyle = ISO, DMY)
> <=BE ParameterStatus(is_superuser = off)
> <=BE ParameterStatus(server_version = 7.4.1)
> <=BE ParameterStatus(session_authorization = oliver)
> <=BE BackendKeyData(pid=676,ckey=704988999)
> <=BE ReadyForQuery(I)
> compatible = 7.5
> loglevel = 0
> prepare threshold = 0
> getConnection returning driver[className=org.postgresql.Driver,org(dot)postgresql(dot)Driver(at)ad3ba4]
> simple execute, handler=org(dot)postgresql(dot)jdbc2(dot)AbstractJdbc2Statement$StatementResultHandler(at)1dd7056, maxRows=0, fetchSize=0, flags=21
> FE=> Parse(stmt=null,query="DECLARE c CURSOR WITH HOLD FOR SELECT typname,oid from pg_type WHERE typname LIKE $1",oids={25})
> FE=> Bind(stmt=null,portal=null,$1=<%>)
> FE=> Describe(portal=null)
> FE=> Execute(portal=null,limit=1)
> FE=> Sync
> <=BE ParseComplete [null]
> <=BE BindComplete [null]
> <=BE NoData
> <=BE CommandStatus(DECLARE CURSOR)
> <=BE ErrorMessage(ERROR: no value found for parameter 1
> Location: File: execQual.c, Routine: ExecEvalParam, Line: 518
> ServerSQLState: 42704)
> java.sql.SQLException: ERROR: no value found for parameter 1
> Location: File: execQual.c, Routine: ExecEvalParam, Line: 518
> ServerSQLState: 42704
> at org.postgresql.core.v3.QueryExecutorImpl.receiveErrorResponse(QueryExecutorImpl.java:1130)
> at org.postgresql.core.v3.QueryExecutorImpl.processResults(QueryExecutorImpl.java:933)
> at org.postgresql.core.v3.QueryExecutorImpl.execute(QueryExecutorImpl.java:139)
> at org.postgresql.jdbc2.AbstractJdbc2Statement.execute(AbstractJdbc2Statement.java:343)
> at org.postgresql.jdbc2.AbstractJdbc2Statement.executeWithFlags(AbstractJdbc2Statement.java:291)
> at org.postgresql.jdbc2.AbstractJdbc2Statement.executeUpdate(AbstractJdbc2Statement.java:246)
> at TestDeclare.main(TestDeclare.java:11)
> SQLException: SQLState(42704)
> <=BE NoticeResponse(WARNING: AbortTransaction and not in in-progress state
> Location: File: xact.c, Routine: AbortTransaction, Line: 1034
> ServerSQLState: 01000)
> SQLWarning:
> <=BE ReadyForQuery(I)

Oliver Jowett <oliver(at)opencloud(dot)com> writes:
> Logs follow; basically this is issuing a Parse/Bind/Execute for a
> parameterized DECLARE, which blows up with "no value found for parameter
> 1" despite there definitely being one there. (also, that error appears
> on Execute, not Parse/Bind).

This may be fixable --- I'm thinking that the problem has to do with not
passing down parameters from one portal to another in a situation where
they probably should be.

> Any chance of getting this fixed for 7.5?

I'm up to my keister in PITR and nested-xact issues and won't have time
to look at it soon. Do you want to take a look and see if you can find
where the ball is getting dropped?

> Alternatively, if we can get WITH HOLD / SCROLL behaviour in portals
> created by Execute (probably means a protocol change) that works too.

This won't be salable unless we decide we really need a protocol change
to support nested xacts properly. With the change to SAVEPOINT syntax
the motivation for that may have diminished --- in particular I doubt
that we can usefully report a nesting depth, so the urge to fool with
the Z message has diminished greatly (in my mind anyway).

I'd suggest looking into the parameter issue if you want to have
confidence in a fix being available in 7.5.

regards, tom lane

Tom Lane wrote:

>>Any chance of getting this fixed for 7.5?
>
>
> I'm up to my keister in PITR and nested-xact issues and won't have time
> to look at it soon. Do you want to take a look and see if you can find
> where the ball is getting dropped?

Ok, I'll do that.

-O

On Wed, 21 Jul 2004, Oliver Jowett wrote:

> It's going to be fun using anything more than very basic cursors via the
> V3 protocol in the JDBC driver. DECLARE does not work with parameters
> passed via a Parse/Bind combination -- which is how we currently always
> pass parameters when talking V3. I'm reluctant to have two
> implementations of the parameter logic for V3 (one that does direct
> substitution, one that uses Bind) since that's extra unnecessary code
> and a recipe for inconsistent behaviour.

I see where you are going in the WITH HOLD case, I don't see how this
extends to scrollable cursors without other major changes as the Execute
protocol message assumes forward only operation.

Kris Jurka

Kris Jurka wrote:

> I see where you are going in the WITH HOLD case, I don't see how this
> extends to scrollable cursors without other major changes as the Execute
> protocol message assumes forward only operation.

Use FETCH, or a protocol change. If we're using DECLARE anyway, it's not
too much extra pain to teach it about FETCH too.

This is all some time away anyway, I was just verifying my initial
assumptions when I hit the DECLARE problem.

-O

Tom Lane wrote:

[... DECLARE with parameters failing ...]

>>Any chance of getting this fixed for 7.5?
>
>
> I'm up to my keister in PITR and nested-xact issues and won't have time
> to look at it soon. Do you want to take a look and see if you can find
> where the ball is getting dropped?

Here's what I've found so far:

The error occurs where it does because it is a DECLARE CURSOR WITH HOLD
outside an explicit transaction, so the resulting cursor is immediately
materialized when the implicit commit at end of statement happens. This
implicitly fetches all rows, and it's the fetch that fails.

When the DECLARE is enclosed in an explicit transaction, it succeeds,
but then subsequent FETCHes fail with the same error.

DECLARE -> PerformCursorOpen always passes NULL as a parameter list to
PortalStart() on the newly created portal, so no parameters are
available when executing it.

It seems like we should pass the original parameters from execution of
the DECLARE utility portal down through PortalRunUtility ->
ProcessUtility -> PerformCursorOpen, copy them into the newly created
portal's memory context, and pass the copies to PortalStart on the new
portal.

Does that sound about right?

-O

Oliver Jowett <oliver(at)opencloud(dot)com> writes:
> It seems like we should pass the original parameters from execution of
> the DECLARE utility portal down through PortalRunUtility ->
> ProcessUtility -> PerformCursorOpen, copy them into the newly created
> portal's memory context, and pass the copies to PortalStart on the new
> portal.

> Does that sound about right?

Hm. The copying bit bothers me, and I guess after some thought it's
a semantic issue. If you've got

DECLARE c CURSOR FOR SELECT ... WHERE foo = $1;

FETCH 10 FROM c;

it's not very clear when the value of $1 should be supplied. With your
proposal the parameter value would have to be supplied in the Bind
message for the DECLARE CURSOR command. That may be the only way to do
it (certainly I'd not want several successive FETCHes to use different
parameter values), but it still seems a bit weird.

BTW, rather than hacking the parameter list of ProcessUtility,
I'd be inclined to just look at ActivePortal->portalParams in
PerformCursorOpen. (Come to think of it, we could also copy
ActivePortal's sourceText at the PortalDefineQuery step.)

regards, tom lane

Tom Lane wrote:

> Hm. The copying bit bothers me, and I guess after some thought it's
> a semantic issue. If you've got
>
> DECLARE c CURSOR FOR SELECT ... WHERE foo = $1;
>
> FETCH 10 FROM c;
>
> it's not very clear when the value of $1 should be supplied. With your
> proposal the parameter value would have to be supplied in the Bind
> message for the DECLARE CURSOR command. That may be the only way to do
> it (certainly I'd not want several successive FETCHes to use different
> parameter values), but it still seems a bit weird.

It doesn't seem too weird, we're using Parse/Bind to separate the query
template from concrete parameter values. When the Bind is done, we want
to behave as if executing the DECLARE with actual parameter values
substituted into the query string originally -- that matches the
behaviour of Bind for other queries. So using the parameters to DECLARE
for the constructed portal seems right.

If we were to do it any other way, we'd have to special-case Parse or
Bind for DECLARE to expect some different (zero?) number of parameters
in the Bind. exec_bind_message does a check quite early on that the
number of parameters provided matches the number expected.

> BTW, rather than hacking the parameter list of ProcessUtility,
> I'd be inclined to just look at ActivePortal->portalParams in
> PerformCursorOpen. (Come to think of it, we could also copy
> ActivePortal's sourceText at the PortalDefineQuery step.)

Ok, I'll do that.

-O

Oliver Jowett wrote:

> It seems like we should pass the original parameters from execution of
> the DECLARE utility portal down through PortalRunUtility ->
> ProcessUtility -> PerformCursorOpen, copy them into the newly created
> portal's memory context, and pass the copies to PortalStart on the new
> portal.

I'll also have to add a type Oid to ParamListInfoData to be able to do
the copying of parameters (either that or pass type info around with the
portal which seems nasty).

-O

Tom Lane wrote:

> BTW, rather than hacking the parameter list of ProcessUtility,
> I'd be inclined to just look at ActivePortal->portalParams in
> PerformCursorOpen. (Come to think of it, we could also copy
> ActivePortal's sourceText at the PortalDefineQuery step.)

I've done this and it seems to work fine for the V3 protocol case.

However there are some callers of ProcessUtility that do not set
ActivePortal: SPI and the SQL function code. These suffer from
essentially the same problem as with the V3 protocol, for example:

> test=# create function f1(text) returns void as $$
> test$# declare c cursor for select * from pg_type where typname like $1;
> test$# $$ language sql;
> CREATE FUNCTION
> test=# begin;
> BEGIN
> test=# select f1('%');
> f1
> ----
>
> (1 row)
>
> test=# fetch all from c;
> ERROR: no value found for parameter 1

SPI has a similar path via SPI_execp -> _SPI_execute_plan.

So it looks like I'll have to go back to passing a parameter list to
ProcessUtility; even if we don't need to support the SPI/SQL function
cases, it seems that ActivePortal is not guaranteed to point to the
right parameter values.

-O