On 3/14/23 12:07, gkokola...@pm.me wrote: > > > ------- Original Message ------- > On Monday, March 13th, 2023 at 10:47 PM, Tomas Vondra > <tomas.von...@enterprisedb.com> wrote: > > > >> >>> Change pg_fatal() to an assertion+comment; >> >> >> Yeah, that's reasonable. I'd even ditch the assert/comment, TBH. We >> could add such protections against "impossible" stuff to a zillion other >> places and the confusion likely outweighs the benefits. >> > > A minor note to add is to not ignore the lessons learned from a7885c9bb. > > For example, as the testing framework stands, one can not test that the > contents of the custom format are indeed compressed. One can infer it by > examining the header of the produced dump and searching for the > compression flag. The code responsible for writing the header and the > code responsible for actually dealing with data, is not the same. Also, > the compression library itself will happily read and write uncompressed > data. > > A pg_fatal, assertion, or similar, is the only guard rail against this > kind of error. Without it, the tests will continue passing even after > e.g. a wrong initialization of the API. It was such a case that lead to > a7885c9bb in the first place. I do think that we wish it to be an > "impossible" case. Also it will be an untested case with some history > without such a guard rail. >
So is the pg_fatal() a dead code or not? My understanding was it's not really reachable, and the main purpose is to remind people this is not possible. Or am I mistaken/confused? If it's reachable, can we test it? AFAICS we don't, per the coverage reports. If it's just a protection against incorrect API initialization, then an assert is the right solution, I think. With proper comment. But can't we actually verify that *during* the initialization? Also, how come WriteDataToArchiveLZ4() doesn't need this protection too? Or is that due to gzip being the default compression method? > Of course I will not object to removing it, if you think that is more > confusing than useful. > Not sure, I have a feeling I don't quite understand in what situation this actually helps. regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
