On 27.01.22 00:32, Tomas Vondra wrote:
>
> On 1/26/22 14:01, Petr Jelinek wrote:
>> I would not remove it altogether, there is plenty of consumers of this
>> extension's output in the wild (even if I think it's
>> unfortunate) that might not be interested in sequences, but changing
>> it to off by default certainly makes sense.
>
> Indeed. Attached is an updated patch series, with 0003 switching it to
> false by default (and fixing the fallout). For commit I'll merge that
> into 0002, of course.
(could be done in separate patches too IMO)
test_decoding.c uses %zu several times for int64 values, which is not
correct. This should use INT64_FORMAT or %lld with a cast to (long long
int).
I don't know, maybe it's worth commenting somewhere how the expected
values in contrib/test_decoding/expected/sequence.out come about.
Otherwise, it's quite a puzzle to determine where the 3830 comes from,
for example.
I think the skip-sequences options is better turned around into a
positive name like include-sequences. There is a mix of "skip" and
"include" options in test_decoding, but there are more "include" ones
right now.
In the 0003, a few files have been missed, apparently, so the tests
don't fully pass. See attached patch.
I haven't looked fully through the 0004 patch, but I notice that there
was a confusing mix of FOR ALL SEQUENCE and FOR ALL SEQUENCES. I have
corrected that in the other attached patch.
Other than the mentioned cosmetic issues, I think 0001-0003 are ready to go.