Hi list,

Free Pascal Compiler version 2.5.1 [2011/08/04] for i386

While improving my fcl-db XML export code, I'm testing with ftVariant
types in a bufdataset:
  FieldDef := FTestDataset.FieldDefs.AddFieldDef;
  FieldDef.Name := 'ftVariant';
  FieldDef.DataType := ftVariant;
This works:
  TestString:='The answer to life, the universe, and everything';
  FTestDataset.Append;
  FTestDataSet.FieldByName('ftVarBytes').AsString:=TestString;
  FTestDataSet.FieldByName('ftVarBytes').SetData(TempPChar);
  //FTestDataSet.FieldByName('ftVariant').AsString:=TestString;
  FTestDataset.Post;

However, I can't assign values to ftVariant, whatever combination I try. The strange thing is that I get an access violation when printing the fields' contents in a loop, and it happens when getting the value for ftVarBytes.
Would it have to do with the ASCII NULL delimiter Ludo Brands reported in http://bugs.freepascal.org/view.php?id=19922

This doesn't work:
  //FTestDataSet.FieldByName('ftVarBytes').AsString:=TestString;
  FTestDataSet.FieldByName('ftVarBytes').SetData(TempPChar);
  FTestDataSet.FieldByName('ftVariant').AsString:=TestString;
nor this:
  //FTestDataSet.FieldByName('ftVarBytes').AsString:=TestString;
  //FTestDataSet.FieldByName('ftVarBytes').SetData(TempPChar);
  FTestDataSet.FieldByName('ftVariant').AsString:=TestString;
nor this:
  FTestDataSet.FieldByName('ftVarBytes').AsString:=TestString;
  FTestDataSet.FieldByName('ftVarBytes').SetData(TempPChar);
  FTestDataSet.FieldByName('ftVariant').AsString:=TestString;

How can I properly assign values to ftVarBytes and ftVariant and retrieve them?
I know I asked before for ftVarBytes; I just use .AsString:='bla', but obviously it doesn't work.

Thanks,
Reinier

Same problem as before: TBufDataset doesn't support correctly ftVarBytes and ftVariant and doesn't raise an SErrFieldTypeNotSupported like TMemDataset is doing.
and you'll see that data length for
ftVarBytes and ftVariant is arbitrarely this with TMemDataset.MDSGetBufferSize.

Ludo

I'm afraid you'll have to spell it out for me; I now set the size
explicitly:
  FieldDef.Name := 'ftVariant';
  FieldDef.DataType := ftVariant;
  FieldDef.Size:=NumberOfBytes;
even I do this:
  FTestDataset.Fieldbyname('ftTypedBinary').Asstring :=
PChar(AnsiLeftStr(TestString,7)+#0);
FTestDataSet.FieldByName('ftVarBytes').AsString:=PChar(AnsiLeftStr(TestString,7)+#0);
FTestDataSet.FieldByName('ftVariant').AsString:=PChar(AnsiLeftStr(TestString,7)+#0);

I still get the errors.

As for patching TBufDataset: ftVariant and ftVarbytes seem like blob-type fields to me, so:
in TCustomBufDataset.GetFieldSize

  case FieldDef.DataType of

...
ftBlob,
ftMemo,
ftGraphic,
ftFmtMemo,
ftParadoxOle,
ftDBaseOle,
ftTypedBinary,
ftOraBlob,
ftOraClob,

      ftWideMemo : result := sizeof(TBufBlobField)
  else Result := 10
  end;

would it make sense to add ftVariant, ftVarBytes to the list with ftBlob etc?
(I'm going to try anyway, see what happens ;)

Thanks for all the help,
Reinier

TBufDataset doesn't support ftVarBytes and ftVariant. Variants are complex objects and aren't stored by just copying bytes. Don't use them with TBufDataset.

Ludo

Thanks, that's very clear - I'll get rid of them in the tests ;)

I might upload a patch to give an error instead of the fixed size - that might possibly lie within my abilities...

Thanks,
Reinier

... cause they have some kind of structure that describes what kind of data the variable/field actually contains and a pointer to memory containing the actual data? Just guessing...

Anyway, once again thanks a lot for the help, I've updated my test code and posted
http://bugs.freepascal.org/view.php?id=19930

Reinier

Yes, a pointer or the actual data for basic types. Sizeof(variant) is 16. Copying only 10 bytes is clearly not working, even for the simple datatypes.

Ludo

I created a patch for ftVariant, ftBytes and ftVarBytes support as well as a check for supported types and attached it to the issue. Feel free to extend your tests with it ;)

Also a change in fpXMLXSDExport line 388:

      Fanode := Foutputdoc.Createtextnode(Utf8decode(EF.Field.AsString));

Instead of

      Fanode :=
Foutputdoc.Createtextnode(Encodestringbase64(EF.Field.AsString));

The format was defined as string before.

Ludo

Ludo Brands wrote

I created a patch for ftVariant, ftBytes and ftVarBytes support as well as a check for supported types and attached it to the issue. Feel free to extend your tests with it ;)

Thanks, I'll have a look.

That doesn't match my current version.
Maybe I already made the change:
I've changed ftVariant to base64 encoding (i.e. treat is as binary) in commit 19:
https://bitbucket.org/reiniero/fpc_laz_patch_playground/changeset/a78b5a5de22e

Can you tell me what field type it is? I'm guessing ftVariant...

You might also be interested in the newest version on Bitbucket. The last couple of days I've added some fixes to block invalid date ranges for Access/Excel, as well as the order of output in Delphi clientdataset... The tests are crashing again now, so I might be back with some newbie questions.

Thanks for the help & the patch,
Reinier

Mmm... That's taking risks and requires quite some testing with the different host applications. When transferring data as strings, we know that apps do the string to integer/date/whatever transformation. But when you transfer them as binary, I'm afraid that they will be imported as is ie. no transformation.

Can you tell me what field type it is? I'm guessing ftVariant...

Yes, it came up when testing new the ftVariant.

Ludo

Yes, it is taking a risk. However, if I understand correctly, a variant can contain an integer, string, anything really. Couldn't it also contain blob or binary data?
Encoding it as base64 seems safer to me than just blindly writing it out as a string.

(Don't know if ftVariant fields are ever used in real world databases though)

I really don't care myself which way we go - I'll happily defer to your expertise.

Just say the word & I'll change them all back to string ;)

Ludo Brands wrote

I created a patch for ftVariant, ftBytes and ftVarBytes support as well as a check for supported types and attached it to the issue. Feel free to extend your tests with it ;)

Applied patch in fpc source, make clean, make all make install for FPC, make for Lazarus, rescan fpc source directory, recompile test project.

Still get access violations.
(ftVariant, ftVarbytes set to size 10, then assigning all kinds of strings to it using .AsString)

Could you have a look at the test code now - I might still be doing something really stupid.
The commit where the files are readded, you can download the test code there:
https://bitbucket.org/reiniero/fpc_laz_patch_playground/changeset/6be7ed0edb4b

(It seems getting the value from the ftVariant field gives an access violation, but I'm not sure)

Thanks,
Reinier

Some output from the test:
*** Starting to fill row 1
<snip>
Field: ftString_256 has value Douglas Adams less than: < greater than

tab: crlf:

åœ‹ç¼ºç•Œå¹¿æ¬ å»£ç•Œç•Œä¸œç¼º. Haddock drinks rosé (ros, e accent aigu), водка (wodka cyrillic) and οÏ�ζο (ouzo Greek) but prefers Loch Lomond whiskey.
Field: ftTime has value 23:59:59
Field: ftTypedBinary has value
Field: ftVariant: error retrieving value:
EAccessViolation; detailed error message: Access violation Field: ftVarBytes has value Douglas Ad
<snip>
EAccessViolation; detailed error message: Access violat Time:00.027 N:8 E:7 F:0 I:0
Ttestxmlxsdexport1 Time:00.027 N:8 E:7 F:0 I:0
00.003 TestXSDExport_Access_NoXSD_Decimal Error: EAccessViolation Exception: Access violation
Source unit:
Method name:
Line number: 574
00.003 TestXSDExport_Access_NoXSD_NoDecimal Error: EAccessViolation Exception: Access violation
Source unit:
Method name:
Line number: 574
00.004 TestXSDExport_Access_XSD_Decimal Error: EAccessViolation Exception: Access violation
Source unit:
Method name:
Line number: 574
00.004 TestXSDExport_Access_XSD_NoDecimal Error: EAccessViolation Exception: Access violation
Source unit:
Method name:
Line number: 574
00.003 TestXSDExport_ADONET_NoXSD Error: EAccessViolation Exception: Access violation
Source unit:
Method name:
Line number: 574
00.003 TestXSDExport_ADONET_XSD Error: EAccessViolation Exception: Access violation
Source unit:
Method name:
Line number: 574
00.003 TestXSDExport_DelphiClientDataset Error: EAccessViolation Exception: Access violation
Source unit:
Method name:
Line number: 574
00.004 TestXSDExport_Excel

Number of run tests: 8
Number of errors: 7
Number of failures: 0

List of errors:
Error:
Message:
Ttestxmlxsdexport1.TestXSDExport_Access_NoXSD_Decimal: Access violation Exception class: EAccessViolation
Exception message: Access violation
Source unitname:
Line number: 574
Failed methodname:

Error:
Message:
Ttestxmlxsdexport1.TestXSDExport_Access_NoXSD_NoDecimal: Access violation Exception class: EAccessViolation
Exception message: Access violation
Source unitname:
Line number: 574
Failed methodname:

Error:
Message:
Ttestxmlxsdexport1.TestXSDExport_Access_XSD_Decimal: Access violation Exception class: EAccessViolation
Exception message: Access violation
Source unitname:
Line number: 574
Failed methodname:

Error:
Message:
Ttestxmlxsdexport1.TestXSDExport_Access_XSD_NoDecimal: Access violation Exception class: EAccessViolation
Exception message: Access violation
Source unitname:
Line number: 574
Failed methodname:

Error:
Message: Ttestxmlxsdexport1.TestXSDExport_ADONET_NoXSD: Access violation
Exception class: EAccessViolation
Exception message: Access violation
Source unitname:
Line number: 574
Failed methodname:

Error:
Message: Ttestxmlxsdexport1.TestXSDExport_ADONET_XSD: Access violation
Exception class: EAccessViolation
Exception message: Access violation
Source unitname:
Line number: 574
Failed methodname:

Error:
Message:
Ttestxmlxsdexport1.TestXSDExport_DelphiClientDataset: Access violation Exception class: EAccessViolation
Exception message: Access violation
Source unitname:
Line number: 574
Failed methodname:

Downloaded the code and it raises an SUnsupportedFieldType when creating the ftAutoInc field. That is OK since I didn't include ftAutoinc in the supported list for TBufDataset. If you are not getting this error, then the patch and/or install didn't work.

Line 547 is where FillTestData is called. Not very usefull.

I do get a segfault in the last test when assigning to ftWideString256. When tracing the code it crashes when moving the size+1 chracters to the database. This fixed length moving is fundamentally wrong. The string is only 20 or so chars long and TCustomBufDataset.SetFieldData tries to do a move of the full 514 bytes (256+1)*2. This asking for trouble. The source of the data is here a temporary WideString created by casting a string, so probably on the heap. Reading 514 bytes when only 40 odd where allocated is not always working....

I'll try to fix that one also.

Ludo

I feared as much. Totally strange, as I tried my patch for SQLExport as well and that did work fine.
I'll clean up everything & try again...

Line 547 is where FillTestData is called. Not very usefull.

I agree. Seems like the test setup & teardown should be a test of their own for TBufDataset :(

I'd appreciate that a lot!

Thanks a lot for the help,
Reinier

line 547 or 574? the error report is 574 ;)

waldo, thanks for trying to help, unfortunately, I think he meant 574 ;)

FillTestData is called in 573; the next line is the end; of the Setup procedure

I probably will need to write a test case for TBufdataset if that hasn't already been done - I know there's some changes going on in the fcl-db test set...

Thanks,
Reinier

> line 547 or 574? the error report is 574 ;) waldo, thanks for trying to help, unfortunately, I think he meant 574 ;)

My keybroad is suffering from dyslexia.

Lduo

so do i but someone had to point out the possible error ;)

mine does it all the time, too :lol:

Ludo Brands wrote

> Applied patch in fpc source, make clean, make all make install for > FPC, make for Lazarus, rescan fpc source directory, recompile test > project.

<snip>

Oops, sorry, you're right. Your cygwin diffs don't seem to agree with (the way I use) fpc patch; I got it to apply only the first part of the patch...
Now it works.

Using fpc diff -u --minimal oldfile newfile > patch.diff does work for me with patch -p0 < patch.diff
I suppose you use cygwin patch to apply the patches?

Very glad the test is working again, now I can get back to improving the export code!

Thanks a million!

Oops, sorry, you're right. Your cygwin diffs don't seem to agree with (the way I use) fpc patch; I got it to apply only the first part of the patch... Now it works.

The patches I upload to bugs.freepascal are made with svn diff. The ones I sent you earlier were made with cygwin diff since the code wasn't versioned with svn.

Using fpc diff -u --minimal oldfile newfile > patch.diff does work for me with patch -p0 < patch.diff I suppose you use cygwin patch to apply the patches?

I use patch -p0 < patch.diff.
The cygwin diff I sent earlier: I forgot to mention the diff -u parameter, hence the different format. Sorry,

Ludo

Uploaded a patch to http://bugs.freepascal.org/view.php?id=19930. Initially, because of the crash, I wanted to submit another bug report but the more I dig into this, the more I feel the definition of SetFieldData/GetFieldData without a length/size parameter and strings passed as pchar is causing all kind of problems:
1) TStringField.SetAsString copies the string to a buffer with size dsMaxStringSize so that datasets that don't figure out the original length of the string can simply copy the full Field.TDatasize (TBufDataset, TDbf, TMemDataset,...). TWideStringField.SetAsString didn't which caused the crash when the string is shorter than Field.Size. TCustomSqliteDataset uses a StrNew(PChar(Buffer)); to get the length of the string but fails sometimes (see 4).
2) dsMaxStringSize isn't enforced for TStringField. Defining a Field.size > dsMaxStringSize causes a crash in TStringField.SetAsString. I haven't raised an issue on this, yet.
3) some of the speed advantage of memory based datasets is offset by moving full Field.Tdatasize bytes in both set and get fielddata
4) pascal strings can contain #0 characters in the string. When converting to pchar part of these strings is lost.

What I propose is:
1 to create overloaded versions of SetFieldData and GetFieldData that include a length parameter, to change TStringField.SetAsString and TWideStringField.SetAsWideString to use these versions and to migrate the different datasets to use these new versions. This way existing (user) code using SetFieldData/GetFieldData will still work (and the user still being responsible for buffer overruns...) and datasets can correctly save and retrieve strings while improving performance.
2 change the bufdataset internal storage for ftstring and ftwidestring fields to include the string length so that stored strings can be retrieved in full when containing #0 characters and without having to copy systematically datasize bytes. However this will introduce an incompatible binary format for TFpcBinaryDatapacketReader. Having a second FpcBinaryIdent would allow for both formats to co-habitate.

If need be the discussion can be moved to fpc-devel.

Ludo

PS: no misunderstanding. I'm volunteering to make these changes;)

MSEgui version of tbufdataset uses UnicodeString in recordbuffer for string fields, see file lib/common/db/msebufdataset.pas.

Martin

MSEgui version of tbufdataset uses UnicodeString in recordbuffer for string fields, see file lib/common/db/msebufdataset.pas.

Well done.

Ludo

BTW. Just noticed http://bugs.freepascal.org/view.php?id=19940 which is directly linked to problem 2).

Ludo

Thanks, no problem.

I'd be happy to help with testing at least!

> What I propose is:

....

Sorry mate. No reaction. Everybody seems to be happy with the current implementation. Under these circumstances, I'm not going to spend any time on creating a patch that makes a, minor, change to the interface when other, less intrusive, patches for fcl-db are sitting in mantis for months/years without being merged.

Ludo

In our previous episode, Ludo Brands said:

Your remarks really hurt my motivation to look at those and the fact that I spent a holiday week closing quite some goes unnoticed.

Marco, all my excuses if you felt my message suggested I was pointing in your direction. I wasn't. I'm a little surprised by you taking this personnally since most of the patches I personnaly submitted and that you dealt with where comitted quickly. And, you don't have to believe me, but I did notice your great efforts last weeks working and catching up on older and new issues.
What did trigger my reaction are examples like this: http://bugs.freepascal.org/view.php?id=19902 a patch contributed recently to implement ODBC transactions while this patch
http://bugs.freepascal.org/view.php?id=14944 was submitted in 2009 for the same but never committed.

Ludo

Marco & Ludo,

Just wanted to say thanks for answering my questions, helping with testing and coding of my export code, the bufdataset patch and, Marco, closing a lot of patches (yes, I noticed, too).
It has really helped improve my knowledge and I think improved the XML export code as well, which is almost (warning: developer's estimate!) ready for testing and then submission as a new patch for FPC.

I noticed the ODBC transaction patches too and it seems a shame. However, it does take experienced people with an overview of the code to review & commit patches, so I can understand these things do happen - nobody is getting paid to work on FPC I suppose.

Regards,
Reinier

In our previous episode, Ludo Brands said:

First, the message came out a bit harsher than I meant it to be. Still it is not fun to read a comment like that after working on something for over a week.

It happens. I can imagine some TDBF bug submitters being quite frustrated too. Negative sentiment is not even limited to fcl-db:

http://zeos.firmos.at/viewtopic.php?t=3157&start=0&postdays=0&postorder=asc&highlight=

Anyway, still it is important to realize that committers are volonteer, and have their own responsibilities too. Preferably test(which is harder for databases), but at least have an idea about what they are committing.