Message-ID: <d0e077e9-6440-fcf5-6b8a-b2a32be408e1@enterprisedb.com>
Date: Wed, 8 Dec 2021 10:04:37 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0)
 Gecko/20100101 Thunderbird/91.3.2
Subject: Re: logical decoding and replication of sequences
Content-Language: en-US
To: Tomas Vondra <tomas.vondra@enterprisedb.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
 Petr Jelinek <petr.jelinek@enterprisedb.com>
References: <d045f3c2-6cfb-06d3-5540-e63c320df8bc@enterprisedb.com>
 <377c6fda-c3eb-7a69-e981-c77ae459c5db@enterprisedb.com>
 <bb890134-a566-efbc-f418-cd0f92eb4b70@enterprisedb.com>
 <5105c3ad-d726-fc22-1d09-514b6852a15e@enterprisedb.com>
 <CALDaNm3gZZjBKWs7gufQep9ikJMtoY9Xv4CsYTpYRaT720RpbQ@mail.gmail.com>
 <3005299b-267d-8e4a-68a3-abdad3e96aba@enterprisedb.com>
 <3d6df331-5532-6848-eb45-344b265e0238@enterprisedb.com>
 <482c10c1-b110-819f-b0e7-17549fa2f0dc@enterprisedb.com>
 <12a9ecd6-4edb-a367-33da-4e2d03b0aace@enterprisedb.com>
 <CAMT0RQQeDR51xs8zTa25YpfKB1B34nS-Q4hhsRPznVsjMB_P1w@mail.gmail.com>
 <bdc688ef-9339-8234-d786-409f1e603711@enterprisedb.com>
 <e6ef8862-31e4-1582-9274-f8f57b7f1cab@enterprisedb.com>
 <2cd38bab-c874-8e0b-98e7-d9abaaf9806a@enterprisedb.com>
 <c0bd317a-fef5-9ef9-66be-a10eae686b96@enterprisedb.com>
 <118b249f-e4ef-2faf-8e25-dd25236dc964@enterprisedb.com>
From: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
In-Reply-To: <118b249f-e4ef-2faf-8e25-dd25236dc964@enterprisedb.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: 
 <https://www.postgresql.org/message-id/d0e077e9-6440-fcf5-6b8a-b2a32be408e1%40enterprisedb.com>
Precedence: bulk

On 08.12.21 01:23, Tomas Vondra wrote:
>> The argument "create" for fill_seq_with_data() is always true (and
>> patch 0002 removes it).  So I'm not sure if it's needed.  If it is, it
>> should be documented somewhere.
>>
> 
> Good point. I think it could be removed, but IIRC it'll be needed when
> adding sequence decoding to the built-in replication, and that patch is
> meant to be just an implementation of the API, without changing WAL.
> 
> OTOH I don't see it in the last version of that patch (in ResetSequence2
> call) so maybe it's not really needed. I've kept it for now, but I'll
> investigate.

Ok, please check.  If it is needed, perhaps then we need a way for 
test_decoding() to simulate it, for testing.  But perhaps it's not needed.

>> The order of the new fields sequence_cb and stream_sequence_cb is a
>> bit inconsistent compared to truncate_cb and stream_truncate_cb.
>> Maybe take another look to make the order more uniform.
>>
> 
> You mean in OutputPluginCallbacks? I'd actually argue it's the truncate
> callbacks that are inconsistent - in the regular section truncate_cb is
> right before commit_cb, while in the streaming section it's at the end.

Ok, that makes sense.  Then leave yours.

When the question about fill_seq_with_data() is resolved, I have no more 
comments on this part.