Fixed in #21305 (and in a little bit better way), same for all other writers

And BTW finalize() is a public part of interface (and this is a why it does not shows the conflicts partially)

This is a good fix as well, but as I see write buffers shall pass finalize() to underlying write buffer anyway.

Indeed, there should be finalize(), not next() (here

Can you send a separate patch for this?

I do not agree, and finish() there shall be called from finalize(), not a destructor.

I see, we shall call finalize() of underlying buffer from finish(), but finish() shall be the same thing as finalize actually.

#22147

But this will hide write error in case of exception, the idea of #19451 is to move such code into the callers (although it is quite a challenge)

Unfortunately, I don't get this change, did you find a way to process two exceptions simultaneously? Is std::terminate a better thing than logging here?

Is std::terminate a better thing than logging here?

Yes, since ignoring write error is unacceptable, it is better to terminate.

Basically it should not be possible if all callers calls finalize(), but if some code path will miss it, then the error may be ignored and the user will not get any error (yes there will be an error in error log, but this is not enough)

But why it does not call finalize() anymore? AFAIU it should call finalize() not next()?

@azat As I understand, finalize() is called before query returns to client, but this call was forgotten when we made compression argument, and behavior of table functions with and without compression became inconsistent:

cristal :) insert into table function url('https://s3.us-west-2.amazonaws.com/altinity-qa-testa/data/', 'CSVWithNames', 'd UInt64', 'deflate') SELECT * from numbers(1)

INSERT INTO FUNCTION url('https://s3.us-west-2.amazonaws.com/altinity-qa-testa/data/', 'CSVWithNames', 'd UInt64', 'deflate') SELECT *
FROM numbers(1)

Query id: ca24b2f0-b79e-4a60-91f0-85a7caf9bf66

Ok.

0 rows in set. Elapsed: 0.736 sec. 

cristal :) insert into table function url('https://s3.us-west-2.amazonaws.com/altinity-qa-testa/data/', 'CSVWithNames', 'd UInt64') SELECT * from numbers(1)

INSERT INTO FUNCTION url('https://s3.us-west-2.amazonaws.com/altinity-qa-testa/data/', 'CSVWithNames', 'd UInt64') SELECT *
FROM numbers(1)

Query id: 71a4779b-8da4-4e18-ab21-dd9abac1b1f0

→ Progress: 1.00 rows, 8.00 B (1.20 rows/s., 9.63 B/s.)  99%
0 rows in set. Elapsed: 0.939 sec. 

Received exception from server (version 21.2.1):
Code: 86. DB::Exception: Received from localhost:9000. DB::Exception: Received error from remote server /altinity-qa-testa/data/. HTTP status code: 405 Method Not Allowed, body: <?xml version="1.0" encoding="UTF-8"?>
<Error><Code>MethodNotAllowed</Code><Message>The specified method is not allowed against this resource.</Message><Method>POST</Method><ResourceType>OBJECT</ResourceType><RequestId>M6Q1NHCWGG0F0SD0</RequestId><HostId>EGOFX9bG3aMMVswqXtCTYPJoIQ/jFHOo2AG9d0JgjdvLF+RNYK9TiQCeLgaAAg8OyKFAp8Z0iqg=</HostId></Error>. 

Previously, I fixed that with these lines of code, calling finalize() in destructor. But it is really a bad place to do so, because it involves lots of initialization code and exceptions in destructor, which shall be catch. finalize() of these write buffers is called at writeSuffix() of corresponding output stream.

As I understand, finalize() is called before query returns to client

It should, can you give a stacktrace example?

but this call was forgotten when we made compression argument, and behavior of table functions with and without compression became inconsistent

Does #21305 fixes it?

Yes it does.

I'll copy tests to another PR.

But why do you removed finish()? What if someone will forget to call finalize()?