Re: logical decoding and replication of sequences

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