zMemtable

[aehimself]

Michal,

Don't use it. I don't know if the size is reported incorrectly or we are typecasting the wrong pointer in GetData. I'll leave this fix to Michael, he has more knowledge about this than I do.

[miab3]

Hi aehimself,

The 'kjteng' version seems to be better at ftDate and works with Lazarus 2.0.12-Win64. But it also has a problem with inserting zeros.
You could take a peek, maybe you can correct it. I send.

ZMemTablemyk3.zip

Michał

[aehimself]

Michal,

The error which is causing the zero date is not in the SaveToStream method, but misaligned data size reported and actual data size. The solution resides in TZDateField & TZTimeField or TZAbstractRODataSet. We actually found a bug, I just don't know which way I should fix it - that's why I am waiting for Michael.

and works with Lazarus 2.0.12-Win64

I asked several times for feedback on Lazarus and as there was none I considered it fully functional.
Does your comment mean that the current in-place implementation does not work on Lazarus?

Once we consider it stable-enough and I get rid of the compiler directive I can modify the test suite to have stream testing.

[miab3]

Hi aehimself,

Does your comment mean that the current in-place implementation does not work on Lazarus?

It works, but with a (critical) Date error which is not present in the uploaded 'kjteng' version (it only has a problem with zeroing empty numbers).
Therefore, I asked for a correction as an alternative solution.

ADD: Your version also has a problem with WIDEMEMO in Lazarus.

Michał

[marsupilami]

It's a .GetData bug, probably like at Blobs. Use ftDateTime instead, it doesn't seem to be affected. I'll look into it but I won't touch anything else than MemTable until Michael confirms if this behavior is normal or not.

Edit: Strange but no, it's not the same. ftDate, ftTime and ftDateTime are all coded down correctly, but it does not work.

ZAbstractRODataSet.pas : 2859 (TZAbstractRODataset.GetFieldData)

Code: Select all

                    else PDateTime(Buffer)^ := DT

DT correctly receives it's data but the assignment above does absolutely nothing to Buffer, it stays [0, 0, 0, 0]. The reason is, we are assigning a TDateTime (Double, which is 8 bytes) to a 4 byte buffer. 4 bytes, that is coming from TDateField.GetDataSize, which returns SizeOf(Integer).
This is actual memory corruption, so if Zeos is using DataSize and GetData internally this must be looked at.

Am am summoning Michael here. Are we missing an override in TZDateField...?

Edit-edit: Can be. Overriding TZDateField's (and TZTimeField's) GetDataSize to return this:

Code: Select all

function TZDateField.GetDataSize: Integer;
begin
  Result := SizeOf(TDateTime);
end;

does fix the issue. On the other hand I'm not sure how the internal design of TZFields work, so - as I mentioned - I'm not touching this

Regarding TZDateField.GetDataSize: I assume for Zeos itself it doesn't matter much. One reason for Michaels implementation of TZ*Fields is that they don't use the Delphi record buffer. They use the RowAccessor directly. So these Fields don't need to care about GetDataSize that much, I think.

I think we should test your correction. Using our test suites, we have a good way to test these changes.

[aehimself]

Regarding TZDateField.GetDataSize: I assume for Zeos itself it doesn't matter much. One reason for Michaels implementation of TZ*Fields is that they don't use the Delphi record buffer. They use the RowAccessor directly. So these Fields don't need to care about GetDataSize that much, I think.

That is good news, no memory corruption in Zeos I suppose.
On the other hand Zeos 8 is happy run on D7, which suggests we do care about backwards compatibility. In that case - I think - we should update all related fields of properties we touched.
I mean, just because Zeos doesn't need it, something else might does.

Also, the issue is still present if you set DisableZFields to false on the dataset, which is bad :) DataSize still comes from TDateField, buffer still gets filled by TZAbstractRODataSet.

I think we should test your correction. Using our test suites, we have a good way to test these changes. :)

Sure thing, I can issue a pull request with it. I'm just feeling extra unsafe on grounds I never wondered upon. I'm trying not to break functionality knowingly.

[aehimself]

My GitHub fork contains the commit which reports a larger data size for TZDateField and TZTimeField as Jan suggested. A pull request is also open to merge these changes back so Jenkins can shout at me if this breaks things.

In the mean time, SaveToStream in my fork dumps time fields corretly, so Michal's test case is successful.

I advise to wait for the tests before using it, though.

@ Michal,
Can you please provide some details on the FPC issues? Without being able to test it myself, "issue with widememo" and "critical date" doesn't mean much without symptoms / stack traces.

[miab3]

Hi aehimself,

Now in D10.3.3-Win32 it works in my testing.
In Lazarus 2.0.12-Win64 crashes when trying to SaveToFile for any table with non-empty MEMO field

This is best seen by downloading:
https://sourceforge.net/projects/lazaru ... %202.0.12/
or other:
https://sourceforge.net/projects/lazarus/files/
you run the installer, and after 5-10 minutes you have Lazarus running
And then:
viewtopic.php?f=3&t=44184

ADD: Also works with Delphi 2007

Michał

[miab3]

Also works with Delphi 2007 but sometimes with LoadFromStream the error 'Invalid floating point operation'
pops up and the stop is on:

ZSysUtils.pas:

Code: Select all

procedure DecodeDateTimeToDate(const Value: TDateTime; var Date: TZDate);
begin
  DecodeDate(Value, Date.Year, Date.Month, Date.Day);
  Date.IsNegative := False;
end;

Michał

[aehimself]

Okay, so the thing is, it doesn't matter if I override TZDateField's GetDataSize, my Delphi 7 installation creates a normal TDateField instead.
We need the WITH_GETFIELDCLASS_TFIELDDEF_OVERLOAD directive, which is defined in Zeos.inc under:

{$IF CompilerVersion >= 20} //Delphi 2009+

So it seems everything below Delphi 2009 will still get 4 bytes as DataSize and copy 8 bytes via GetData.

This seems wrong to me. Shouldn't .GetData actually check if we created a TZField, otherwise just use the PInteger version...?!
I bet Lazarus will be the same, therefore the Date issue Michal reported.

[aehimself]

All,

My fork now contains a commit which half-implements kjteng's idea. Now Memo and WideMemo fields are read / written as String, only blobs are saved as streams, the rest is saved with GetData / SetData with an in-place fix for ftDate and ftTime fields.
Please check. Test cases succeed on D7 and D10.4.2.

[miab3]

Hi aehimself,

For D10.3.3-Win32 and LoadFromFile still 'Out of memory' sometimes 'Stream read error'.

Michał

[aehimself]

As I'm getting close to none test cases, I created one for myself.

Code: Select all

procedure TForm1.Button3Click(Sender: TObject);

 Function FieldVal(Const inFieldName: String): Variant;
 Begin
  If inFieldName = 'String' Then Result := 'Testing, Testing'
    Else
  If inFieldName = 'SmallInt' Then Result := 32767
    Else
  If inFieldName = 'Integer' Then Result := 1000000
    Else
  If inFieldName = 'Word' Then Result := 65535
    Else
  If inFieldName = 'Boolean' Then Result := True
    Else
  If inFieldName = 'Float' Then Result := 3.14
    Else
  If inFieldName = 'Date' Then Result := '2011/11/11'
    Else
  If inFieldName = 'Time' Then Result := '6:15'
    Else
  If inFieldName = 'DateTime' Then Result := '2011/11/11 6:15'
    Else
  If inFieldName = 'Blob' Then Result := 1986 // VarArrayOf([19, 86, 10, 15])
    Else
  If inFieldName = 'Memo' Then Result := 'Hello, world!'
    Else
  If inFieldName = 'WideString' Then Result := 'The quick brown fox jumps over the lazy dog'
    Else
  If inFieldName = 'LargeInt' Then Result := 9223372036854775807
    Else
  If inFieldName = 'WideMemo' Then Result := 'WideMemo Test Data'
    Else
  If inFieldName = 'LongWord' Then Result := 4294967295
    Else
  If inFieldName = 'ShortInt' Then Result := 127
    Else
  If inFieldName = 'Byte' Then Result := 255
    Else
  If inFieldName = 'Single' Then Result := 3.14;
 End;

Var
 a: Integer;
begin
  ClearAll;

  ZMemTable1.FieldDefs.Add('String', ftString, 100);
  ZMemTable1.FieldDefs.Add('SmallInt', ftSmallInt);
  ZMemTable1.FieldDefs.Add('Integer', ftInteger);
  ZMemTable1.FieldDefs.Add('Word', ftWord);
  ZMemTable1.FieldDefs.Add('Boolean', ftBoolean);
  ZMemTable1.FieldDefs.Add('Float', ftFloat);
  ZMemTable1.FieldDefs.Add('Date', ftDate);
  ZMemTable1.FieldDefs.Add('Time', ftTime);
  ZMemTable1.FieldDefs.Add('DateTime', ftDateTime);
  ZMemTable1.FieldDefs.Add('Blob', ftBlob);
  ZMemTable1.FieldDefs.Add('Memo', ftMemo);
  ZMemTable1.FieldDefs.Add('WideString', ftWideString, 100);
//  ZMemTable1.FieldDefs.Add('LargeInt', ftLargeInt);
//  ZMemTable1.FieldDefs.Add('WideMemo', ftWideMemo);
//  ZMemTable1.FieldDefs.Add('LongWord', ftLongWord);
//  ZMemTable1.FieldDefs.Add('ShortInt', ftShortInt);
//  ZMemTable1.FieldDefs.Add('Byte', ftByte);
//  ZMemTable1.FieldDefs.Add('Single', ftSingle);

  ZMemTable1.Open;

  ZMemTable1.Append;
  ZMemTable1.Post;

  ZMemTable1.Append;
  For a := 0 To ZMemTable1.FieldCount - 1 Do
   ZMemTable1.Fields[a].Value := FieldVal(ZMemTable1.Fields[a].FieldName);
  ZMemTable1.Post;

  SaveAndReload;

  ZMemTable1.First;
  For a := 0 To ZMemTable1.FieldCount - 1 Do
   If Not ZMemTable1.Fields[a].IsNull Then Raise Exception.Create('Field ' + ZMemTable1.Fields[a].FieldName + ' is not null, however it should be!');

  ZMemTable1.Next;

  For a := 0 To ZMemTable1.FieldCount - 1 Do
   Try
    If ZMemTable1.Fields[a].FieldName <> 'Blob' Then If ZMemTable1.Fields[a].Value <> FieldVal(ZMemTable1.Fields[a].FieldName) Then Raise Exception.Create('Field ' + ZMemTable1.Fields[a].FieldName + ' value error');
   Except
    On E:Exception Do Raise Exception.Create('Field ' + ZMemTable1.Fields[a].FieldName + ' value error: ' + E.Message);
   End;
end;

The code above executed successfully on D7 and D10.4.2 Win32 and Win64. Blobs values are not verified as I couldn't really care to write the CompareMem verification for .AsBytes :)

The source of the newest Stream read error was a damn variable I forgot to change, I don't know how it did not show up in the first two tests.
My fork contains the fix, a pull request was also issued.
Please check and confirm. Thanks.

[miab3]

Hi aehimself,

With your patch from an hour ago(for Zeos8_trunk7715):

L606: - AStream.Read(buf, Length(buf));
+ AStream.Read(buf, fsize);

My test on FirebirdSQL database tables passed for:
- Delphi 2007
- Delphi XE2-Win32/Win64
- Delphi 10.3.3-Win32/Win64
- Delphi 11sp1-Win32/Win64
- Lazarus 2.0.8-Win32
- Lazarus 2.0.12-Win64
- Lazarus 2.2.0RC2-Win64
Uff

Michał

[kjteng]

Hi, aehimself:

I have just downloaded and tested on Lazarus 2.0.12 and have two observations bellow :

1. Line 722 (Savetostream), if we include ftstring in the first case (see below), the stream/file size will reduced significantly
Case field.DataType Of
ftString, ftMemo {$IFDEF WITH_WIDEMEMO}, ftWideMemo{$ENDIF}: WriteString(field.AsString); // <--add ftString
....
(LoadFromStream method also need to be changed accordingly)

2. Line 544, it will be safer to reset the position of Astream before reading the fieldcount (this is because the AStream passed to the method might not be in the 0 position), as follows:
try
AStream.Position := 0; // <- set stream cursor position to 0
// Recreate FieldDefs
len := ReadInt;

Hope you will consider and incorporate the above suggestion.