On 1/27/22 17:05, Peter Eisentraut wrote:
> 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).
>
Good point - INT64_FORMAT seems better. Also, the formatting was not
quite right (missing space between the colon), so I fixed that too.
> 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.
>
Yeah, that's probably a good idea - I had to think about the expected
output repeatedly, so an explanation would help. I'll do that in the
next version of the patch.
> 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.
>
Hmmm. I don't see much difference between skip-sequences and
include-sequences, but I don't feel very strongly about it either so I
switched that to include-sequences (which defaults to true).
> In the 0003, a few files have been missed, apparently, so the tests
> don't fully pass. See attached patch.
>
D'oh! I'd swear I've fixed those too.
> 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.
Thanks. I think we'll have time to look at 0004 more closely once the
initial parts get committed.
Attached is a a rebased/squashed version of the patches, with all the
fixes discussed here.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company