FirebirdSQL / firebird

Firebird server, client and tools
https://www.firebirdsql.org/
1.19k stars 204 forks source link

Bug in FB5 when restoring a database locally. #8092

Closed C-SICARD closed 3 weeks ago

C-SICARD commented 3 weeks ago

Hi, A bug appeared in FB5 when restoring a database locally. The buffer size returned in isc_info_svc_stdin is 0x40000 instead of 0x4000 (16384).

aafemt commented 3 weeks ago

Not a bug. Backup service really uses such buffer. The fact that you cannot send such volume of data is result of SPB structure which can be changed.

aafemt commented 3 weeks ago

Actually value 16385 would be a bug because is less that allowed 32763.

AlexPeshkoff commented 3 weeks ago

Please provide test case - I hardly understand your problem. Use of services mentioned by you does not look well together with "restoring a database locally". Also take into an account that one is not forced to send exactly requested data volume - "not more" is requirement.

AlexPeshkoff commented 3 weeks ago

@aafemt Why bug? If server has such size of buffer it tells client to send not more than it.

aafemt commented 3 weeks ago

Because server responds to client only after previous buffer is (mostly) processed. Returning of 16384 would indicate that server has too small buffer.

PS: Personally I consider whole this design with isc_info_svc_stdin to be wrong. Server must simply accept data stream which client is sending to it and return from the call only when data is processed. Exposing size of internal buffer to the client is "a bad thing". I.e. server must not request data from client, it is not the way client-server is supposed to work.

AlexPeshkoff commented 3 weeks ago

Currently data exchange takes place in parallel with restore. If I understood your suggestion correctly that parallelism will be lost. Pay attention - due to running in parallel there is no need in too big buffer.

aafemt commented 3 weeks ago

If I understood your suggestion correctly that parallelism will be lost.

It depends on implementation. For parallelization it is enough to copy data from received buffer into internal buffer and return from query() call immediately while background process proceedes the internal buffer.

AlexPeshkoff commented 3 weeks ago

When I was implementing it I slightly cared about RAM usage and when possible avoided extra data copy.

Can you explain what danger is exposing internal buffer size? Sending too much data to overflow it is carefully avoided.

aafemt commented 3 weeks ago

There is no danger in exposing of buffer size, it is just not necessary (suboptimal, overcomplicated). Application code might be simpler.

It is not possible to send too much data because in the embedded case the buffer is allocated by client application and in the remote case it is allocated by server and its size is limited by protocol. Extra data copy could be avoided if restore code is working directly with arrived buffer but as you said above it would prevent parallelization so data copy is unavoidable (except server case again where allocated buffer can be reowned instead of copying but that would require some API changes).

C-SICARD commented 3 weeks ago

The problem is that the server requests 0x40000 until it returns an error "isc_gbak_inv_bkup_ver2".

If I transmit 0x4000 bytes, the restoring works fine but it doesn't match the size request returned in isc_info_svc_stdin (0x40000).

If I look at the Firebird source code the buffer size is 16384. With Firebird 2.5, everything is OK.

aafemt commented 3 weeks ago

The problem is that the server requests 0x40000 until it returns an error "isc_gbak_inv_bkup_ver2".

These things are not related. Your code must do something wrong because size of chunk in SPB is two bytes so you physically cannot send more than 65535 bytes of backup data. And it is not related to requested buffer size in any way.

UPD: 65535 bytes of course.

aafemt commented 3 weeks ago

It is not limited to FB5 and local restore. Remote Firebird 3/4 has the same behavior.

C-SICARD commented 3 weeks ago

Source code :

Begin ...
InternalServiceQuery(lInputBuffer, lSendSize); // Transmit data ...

lRunLen := 0; lRequestLength := 0;

While (not (BYTE(OutputBuffer[lRunLen]) = isc_info_end)) do begin case BYTE(OutputBuffer[lRunLen]) of isc_info_svc_stdin :begin Inc(lRunLen);
lRequestLength := isc_vax_integer(OutputBuffer + RunLen, 4); // Return always 0x40000 inc(RunLen, 4); break; end;

        isc_info_svc_line:  begin
                              Inc(lRunLen);
                              lVerbose       := ParseString(lRunLen);
                            end;

    end;                        

end;

aafemt commented 3 weeks ago

Source code

This is code to receive requested data length, not for sending requested data where your bug must be.

AlexPeshkoff commented 3 weeks ago

Yes, buffer in gbak was increased far before FB5. We can limit it to maximum one that fits into current SPB format but that's not good solution cause:

I.e. you program should watch the correct size itself, treat it as part of port from 2.5.