From: | Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com> |
---|---|
To: | Tomas Vondra <tomas(dot)vondra(at)enterprisedb(dot)com>, Petr Jelinek <petr(dot)jelinek(at)enterprisedb(dot)com> |
Cc: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: logical decoding and replication of sequences |
Date: | 2022-01-27 16:05:59 |
Message-ID: | 802f8b1b-a798-be52-c829-7f0fb9faafd5@enterprisedb.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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.
Attachment | Content-Type | Size |
---|---|---|
0001-fixup-Change-skip-sequences-to-false-by-default.patch | text/plain | 5.8 KB |
0003-fixup-Add-support-for-decoding-sequences-to-built-in.patch | text/plain | 1.6 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Robert Haas | 2022-01-27 16:47:47 | Re: refactoring basebackup.c |
Previous Message | Alvaro Herrera | 2022-01-27 15:15:27 | Re: support for MERGE |