Regression: no isc_stream_eof returned by IStatement::execute()

[aafemt]

As reported by @Ev3nt execution of a singleton query returning a data doesn't return isc_stream_eof error if no record was found.

Proposed patch:

diff --git a/src/dsql/DsqlRequests.cpp b/src/dsql/DsqlRequests.cpp
index 17059c7717..dac79823d1 100644
--- a/src/dsql/DsqlRequests.cpp
+++ b/src/dsql/DsqlRequests.cpp
@@ -596,19 +596,28 @@ void DsqlDmlRequest::doExecute(thread_db* tdbb, jrd_tra** traHandle,
 
 			// if this is a singleton select that return some data, make sure there's in fact one record
 
-			if (singleton && (request->req_flags & req_active) && outMsgLength > 0)
+			if (singleton)
 			{
-				// Create a temp message buffer and try one more receive.
-				// If it succeed then the next record exists.
+				if (outMsgLength > 0)
+				{
+					// No record returned though expected
+					if (!(request->req_flags & req_active))
+					{
+						status_exception::raise(Arg::Gds(isc_stream_eof));
+					}
 
-				std::unique_ptr<UCHAR[]> message_buffer(new UCHAR[outMsgLength]);
+					// Create a temp message buffer and try one more receive.
+					// If it succeed then the next record exists.
 
-				JRD_receive(tdbb, request, message->msg_number, outMsgLength, message_buffer.get());
+					std::unique_ptr<UCHAR[]> message_buffer(new UCHAR[outMsgLength]);
 
-				// Still active request means that second record exists
-				if ((request->req_flags & req_active))
-				{
-					status_exception::raise(Arg::Gds(isc_sing_select_err));
+					JRD_receive(tdbb, request, message->msg_number, outMsgLength, message_buffer.get());
+
+					// Still active request means that second record exists
+					if ((request->req_flags & req_active))
+					{
+						status_exception::raise(Arg::Gds(isc_sing_select_err));
+					}
 				}
 			}
 		}

[hvlad]

Why not create PR ?

[hvlad]

Is it about singleton select's only ? I see no reference on singleton select in user message.

[aafemt]

Only select can return zero or more than one record. Stored Procedures have no such ability.

[hvlad]

User say no singleton. It needs explainations, at least, imho
This ticket description is absolutely unclear.

[aafemt]

There was only one case when engine returned isc_stream_eof.

[hvlad]

It adds zero for better understanding of issue

[AlexPeshkoff]

@hvlad Even not diving too deep into details of what user (may be not familiar what means singleton) wrote an issue appears clear to me. Any select raised isc_stream_eof on an attempt to fetch past EOS before FB4. isc_stream_eof was caught in FB3's SRP server code to return isc_nologin error.

Since FB4 server does not raise isc_stream_eof when fetching from regular cursor. Not sure is it good or bad, sooner - bad. That did not break SRP (pay attention - SELECT in it is not singleton!) because as the result of changed API it tried to use empty credentials (like password hash) to validate user, which also caused isc_nologin error.

FB6 currently does not raise isc_stream_eof in singleton select too. This should definitely be fixed, though it's absolutely unrelated with SRP.

What should be discussed: does it make sense to restore FB3 behavior regarding isc_stream_eof when fetching after EOS? On my mind such error is logical here, but not sure how does it feel about SQL standard. Also it can break some application working currently with FB4 & 5. I.e. better leave it as is.

[Ev3nt]

User say no singleton. It needs explainations, at least, imho This ticket description is absolutely unclear.

Ok, for example we have table like this:

CREATE TABLE Users (
    ID INTEGER NOT NULL PRIMARY KEY,
    Login VARCHAR(50) NOT NULL,
    NickName VARCHAR(50),
    Age INTEGER
);

And we need to take the user's nickname from their login:

LocalStatus st;
CheckStatusWrapper status(&st);

Message result;
Field<Varying> nickName(result, 64);

const char* findSql = "SELECT NickName FROM Users WHERE Login = 'Aboba'";

att->execute(&status, tra.get(), 0, findSql, SQL_DIALECT_V6,
			find.getMetadata(), find.getBuffer(), result.getMetadata(), result.getBuffer());

if (st.getState() & IStatus::STATE_ERRORS)
{
	if (fb_utils::containsErrorCode(st.getErrors(), isc_stream_eof))
	{
		gds__log("There are no users with login 'Aboba'");
		return false;
	}

	check(&status);
}

// Another code

We know that there might be a user with the login "Aboba", and we need to know his nickname. It is also important to understand that the user might not have a nickname, so it is best to display an error message if the user does not exist, and otherwise display his login if he does not have a nickname.

Due to the lack of isc_stream_eof, there is no adequate way to check if the request returned anything, so we have to modify the request itself, which affects the resources consumed.

[hvlad]

@hvlad Even not diving too deep into details of what user (may be not familiar what means singleton) wrote an issue appears clear to me. Any select raised isc_stream_eof on an attempt to fetch past EOS before FB4. isc_stream_eof was caught in FB3's SRP server code to return isc_nologin error.

You also doesn't use singleton above, isn't is ?

Also, don't forget that issue's descriptions used to build release notes and many "not familiar" users reads them.
Therefore I prefer to make description more useful for most users, at least in the cases like this one.

Since FB4 server does not raise isc_stream_eof when fetching from regular cursor. Not sure is it good or bad, sooner - bad. That did not break SRP (pay attention - SELECT in it is not singleton!) because as the result of changed API it tried to use empty credentials (like password hash) to validate user, which also caused isc_nologin error.

SRP uses regular SELECT, yes, but it run Statement.execute() with output message specified - it makes it singleton.
And this is another reason why I ask to create more clear decription.

FB6 currently does not raise isc_stream_eof in singleton select too. This should definitely be fixed, though it's absolutely unrelated with SRP.

What should be discussed: does it make sense to restore FB3 behavior regarding isc_stream_eof when fetching after EOS? On my mind such error is logical here, but not sure how does it feel about SQL standard. Also it can break some application working currently with FB4 & 5. I.e. better leave it as is.

[hvlad]

Want to add that @aafemt is not only too lazy to do the things right and useful for someone except of himself.
Often it looks like mockery of readers.

[mrotteveel]

What should be discussed: does it make sense to restore FB3 behavior regarding isc_stream_eof when fetching after EOS? On my mind such error is logical here, but not sure how does it feel about SQL standard. Also it can break some application working currently with FB4 & 5. I.e. better leave it as is.

Wasn't it removed due to scrollable cursors? In a scrollable cursor, selecting beyond the end is absolutely expected and normal behaviour (as an aside, I've never seen nor used isc_stream_eof that I'm aware of).

[sim1984]

@mrotteveel,
IStatement::openCursor or IAttachment::openCursor should not throw any errors. This is about IStatement::execute or IAttachment::execute for which an output message is defined.

[aafemt]

What should be discussed: does it make sense to restore FB3 behavior regarding isc_stream_eof when fetching after EOS?

AFAIK the standard expects returning NO_DATA from fetch in this case, nothing more.

[mrotteveel]

This is not a fetch though.