Close before Open

[Michl]

Hi,

after a day of reduction of a old big app of mine, I was able to figure out, that I forgot a ZQuery.Close before a ZQuery.Open. With Zeos 7.2, there it is no problem, when you don't close the query. In Zeos 8.0 or Trunk, there is the problem, that the query isn't refreshed. I don't know, if this is often asked (I don't find it in a search in forum), but is this change intended?

My environment: Windows 7, Lazarus 2.3.0 r65403 FPC 3.2.0 x86_64-win64-win32/win64, Zeos 7.2 r7597 / Zeos Trunk r7602, PostgreSQL 10

I do:

Code: Select all

  Query.Close;  // <-- this has driven me nuts
  Query.SQL.Text :=
    'SELECT id, sourceid, topicid, ... FROM foo WHERE sourceid = :asourceid ORDER BY id ASC;';
  Query.ParamByName('asourceid').AsInteger := FCurrentSource;
  Query.Open;

[marsupilami]

Hello Michl,

are you sure, this was not an issue with Zeos 7.2? I had similar issues with older versions of Zeos. So usually I would argue, it works as designed. But maybe Egonhugeist has a different opinion or changed that behavior.

Best regards,

Jan

[aehimself]

I agree with Jan. However I was never in this situation, changing the SQL / trying to reopen a dataset while it might still be open sounds like a programming issue.
Use different ZQuery objects / master-detail relations or simply call .Close every time before you .Open. It won't do anything anyway id the dataset is closed already.

[Michl]

Thank you for your answers!

Yes I'm sure, with Zeos 7.2, it was possible to open the query with a active query. (I've set up two Lazarus versions, one with Zeos 7.2, one with Zeos Trunk. As I don't know where the problem came from and the data is not human readable, the debugger wasn't helpfull to see whats going on. So I reduced the app. I still have this test here and can see it working without query close with 7.2, but not with Trunk)

Ok, if this intended, it would be nice, there is a exception raised. This would helped me to save a lot of time.

[aehimself]

This is disturbing indeed, especially if we change the SQL while the dataset is open it will trigger the recreation of parameters.

Easily solved though by overriding the Changing method of TZSQLStrings with:

Code: Select all

procedure TZSQLStrings.Changing;
begin
  If Assigned(FDataSet) And (FDataSet As TZAbstractRODataset).Active Then
    Raise EZSQLException.Create(SResultsetIsAlreadyOpened);

  inherited;
end;

This will throw the usual "Resultset is already open." exception upon any kind of modification. Tested with:

Code: Select all

 ZConnection1.Connect;
 ZQuery1.SQL.Text := 'SELECT 1';
 ZQuery1.Open;

 ZQuery1.SQL.Text := 'SELECT 2'; // Triggers exception
 ZQuery1.SQL.Strings[0] := 'aaa'; // Triggers exception
 ZQuery1.SQL.Clear; // Triggers exception

@Jan & || Michael,
I was lazy adding a new localization string so I reused SResultsetIsAlreadyOpened. Is this message suitable for this situation in your opinion?
Should I prepare the pull request for this?

[marsupilami]

Honestly I wonder why we don't do it the other way around: Close the Dataset if it is open. This can be handy especially if somebody (me) writes an admin app: load the SQL and examine the parameters while we still show the old results.

[aehimself]

I don't know, it sounds unjust.
In a real life scenario let's say you waited 10 minutes to fetch results, and due to a bug in your code you reassign the SQL. Instead of throwing an exception Zeos closes the dataset so you'll have to wait 10 minutes again.

This just sounds too invasive in my opinion.

Then again whatever the decision / fix will be I'll be unaffected as I'm closing my datasets in the moment they are not needed anymore :)

[marsupilami]

I don't know, it sounds unjust.
In a real life scenario let's say you waited 10 minutes to fetch results, and due to a bug in your code you reassign the SQL. Instead of throwing an exception Zeos closes the dataset so you'll have to wait 10 minutes again.

I meant this in a different way. My suggestion is to implement Michls expectation. We do allow to modify SQL and Paramaters and when Open gets called on an open dataset we close it before we open it again. This sounds much more - hmm - organic - to me. I am at a loss for better words. I don't see a benefit in making a call to Close mandatory before repoening a dataset.

In my opinion opening and closing datasets is too complicated anyway - at least for simple queries, which is why I implemented some helpers:

Code: Select all

procedure TZQueryHelper.SelectSQL(const SqlQuery: String; const Parameters: TVariantDynArray);
var
  x: Integer;
begin
  Close;
  SQL.Text := SqlQuery;
  for x := Low(Parameters) to High(Parameters) do begin
    ParamByName('P' + IntToStr(x)).Value := Parameters[x];
  end;
  Open;
end;

procedure TZQueryHelper.ExecuteSQL(const SqlQuery: String; const Parameters: TVariantDynArray);
var
  x: Integer;
begin
  Close;
  SQL.Text := SqlQuery;
  for x := Low(Parameters) to High(Parameters) do begin
    ParamByName('P' + IntToStr(x)).Value := Parameters[x];
  end;
  ExecSQL;
end;

function TZQueryHelper.GetSingletonSelect(const SqlQuery: String; const Paramaters: TVariantDynArray): Variant;
begin
  SelectSQL(SqlQuery, Paramaters);
  try
    if RecordCount <> 1 then begin
      raise Exception.Create('GetSingletonSelect expects a result of exactly one record. ' + IntToStr(RecordCount) + ' records were returned.');
    end else begin
      if FieldCount <> 1 then begin
        raise Exception.Create('GetSingletonSelect expects a result record with exactly one field. ' + IntToStr(RecordCount) + ' fields were returned.')
      end else begin
        Result := Fields[0].Value;
      end;
    end;
  finally
    Close;
  end;
end;

These help me in executing simplistic queries like this:

Code: Select all

var
  SomeSetting: String;
begin
  // some code
  SomeSetting := VarToString(TempQuery.GetSingletonSelect('select value from attributes where attribute = :P0', ['SomeSetting']));
  // more code
end;

I think this greatly simplifies code - write just one line where I would have had to write at least five lines otherwise.

[aehimself]

We do allow to modify SQL and Paramaters and when Open gets called on an open dataset we close it before we open it again.

This is going to cause problems if you want to access a parameter from the previous query (or call .Refresh) after you changed the SQL. We can put safeguards everywhere but my suggestion is still just to throw an exception. It's not hard to call .Close (even better, have a helper like yours) before manipulating with the query text.

[marsupilami]

This is going to cause problems if you want to access a parameter from the previous query (or call .Refresh) after you changed the SQL.

Hmm - i would say that it is the users problem if he tries to use previous paramaters after changing the SQL. I didn't see that there acre cases when Zeos would need the old parameters. But calling refresh is a valid point, I think.

So - yes - a pull request or patch file would be nice

[aehimself]

Although I'm ~900 kms away from home, pull request is available on GitHub :)

Wish me a safe journey back, the weather was not polite at all on the way here.

[MJFShark]

Safe wishes sent!

-Mark

[marsupilami]

Although I'm ~900 kms away from home, pull request is available on GitHub

Thank you for the patch. It seems that a number of tests rely on being able to change the SQL while a query is open. I think we need to fix these tests and need to add a note to the release notes. People might very well rely on these things - as our test suites do.

But I am still not 100% sure if this is the right way to go. I know - I can be quite stubborn

Regarding the SQL and parameters for refresh: Are you sure that the current parameters and the current SQL of a TZQuery get used when doing a refresh? I could imagine that we simply use the parameter set of the IZStatement that belongs to the Query and don't rely on anything else?

Wish me a safe journey back, the weather was not polite at all on the way here.

I hope you got home safely

[aehimself]

Made it back in one piece. Offtopic question: did you guys travel recently and if yes was any entity (border control, hotels, etc.) even interested about or verified the validity of the EU Covid certificates...? Just asking because - surprisingly - I could have gone without any.

It seems that a number of tests rely on being able to change the SQL while a query is open. I think we need to fix these tests and need to add a note to the release notes. People might very well rely on these things - as our test suites do.

I started to add the missing Close statements to the test suite. Will probably take a while, let you know once done.

But I am still not 100% sure if this is the right way to go. I know - I can be quite stubborn ;)

The fact that the tests do this indeed points to the direction that 1, it is a feature and was implemented to be able to do or 2, noone actually thought about it. If you wish roll back the change and wait for Michael to share his opinion on this matter.

Regarding the SQL and parameters for refresh: Are you sure that the current parameters and the current SQL of a TZQuery get used when doing a refresh? I could imagine that we simply use the parameter set of the IZStatement that belongs to the Query and don't rely on anything else?

No, I'm not sure, I did not check the code when I posted this. However there are lots of things to be checked to confirm this (master-detail is one of those) and I'm sure I never used (maybe don't even know the existence of) most of those.

[marsupilami]

Hmm. let me try to write what I think. I will try to differentiate between the reponsibilities of us (Zeos) and a user.

I think that it is the users responsibility to decide when to modify properties. If a user modifies properties, it is the users responsibility to know that he modified properties. So - assigning a new SQL statement and then trying to access the old parameters, that might not exist anymor for me is a use case where the user is responsible to know what he or she does.

I think it is our responsibility to make sure that we can do what we promise to do. When it comes to modifying the current SQL statement, I tink that we should forbid changing it if there is a technical reason to do so. If there is no such technical reason, we should allow modifying the SQL statement.

So for me -> Changing the SQL statement might lead to a new set of parameters. If we need this set of parameters for some reason (doing a refresh, doing something else) we should forbid changing the SQL statement. But if we have a copy of these parameter values on the DBC layer and don't need the values of the old parameter set for anything, then we should not forbid changing the SQL statement.

So - this means to decide wether we should forbid changing the SQL statement or not, we should first know if there is a technical reason to forbid it. But then - if we need the old parameters, we also would have to keep the user from changing the parameters while the query is open.

Another option migt be this one: If somebody changes the SQL statement we could set a flag FSqlHasChanged or something like this. If somebody calls Refresh and FSqlHasChanged is true, we could raise an exception, that says "Sorry, guy - you changed the SQL statement, so we cannot do a refresh."

Does that make sense?

Best regards,

Jan