Re: logical decoding and replication of sequences, take 2

On 7/14/23 09:34, Ashutosh Bapat wrote:
> On Thu, Jul 13, 2023 at 9:47 PM Tomas Vondra
> <tomas.vondra@enterprisedb.com> wrote:
> 
>>
>>>>
>>>> 6) simplified protocol versioning
>>>
>>> I had tested the cross-version logical replication with older set of
>>> patches. Didn't see any unexpected behaviour then. I will test again.
>>>>
>>
>> I think the question is what's the expected behavior. What behavior did
>> you expect/observe?
> 
> Let me run my test again and respond.
> 
>>
>> IIRC with the previous version of the patch, if you connected an old
>> subscriber (without sequence replication), it just ignored/skipped the
>> sequence increments and replicated the other changes.
> 
> I liked that.
> 

I liked that too, initially (which is why I did it that way). But I
changed my mind, because it's likely to cause more harm than good.

>>
>> The new patch detects that, and triggers ERROR on the publisher. And I
>> think that's the correct thing to do.
> 
> With this behaviour users will never be able to setup logical
> replication between old and new servers considering almost every setup
> has sequences.
> 

That's not true.

Replication to older versions works fine as long as the publication does
not include sequences (which need to be added explicitly). If you have a
publication with sequences, you clearly want to replicate them, ignoring
it is just confusing "magic".

If you have a publication with sequences and still want to replicate to
an older server, create a new publication without sequences.

>>
>> There was a lengthy discussion about making this more flexible (by not
>> tying this to "linear" protocol version) and/or permissive. I tried
>> doing that by doing similar thing to decoding of 2PC, which allows
>> choosing when creating a subscription.
>>
>> But ultimately that just chooses where to throw an error - whether on
>> the publisher (in the output plugin callback) or on apply side (when
>> trying to apply change to non-existent sequence).
> 
> I had some comments on throwing error in [1], esp. towards the end.
> 

Yes. You said:

    If a given output plugin doesn't implement the callbacks but
    subscription specifies sequences, the code will throw an error
    whether or not publication is publishing sequences.

This refers to situation when the subscriber says "sequences" when
opening the connection. And this happens *in the plugin* which also
defines the callbacks, so I don't see how we could not have the
callbacks defined ...

Furthermore, the simplified protocol versioning does away with the
"sequences" option, so in that case this can't even happen.

>>
>> I still think it might be useful to have these "capabilities" orthogonal
>> to the protocol version, but it's a matter for a separate patch. It's
>> enough not to fail with "unknown message" on the subscriber.
> 
> Yes, We should avoid breaking replication with "unknown message".
> 
> I also agree that improving things in this area can be done in a
> separate patch, but as far as possible in this release itself.
> 
>>> If the DDL replication takes care of replicating and applying sequence
>>> changes, I think we don't need the changes tracking "transactional"
>>> sequence changes in this patch-set. That also makes a case for not
>>> adding a new field to WAL which may not be used.
>>>
>>
>> Maybe, but the DDL replication patch is not there yet, and I'm not sure
>> it's a good idea to make this patch wait for a much larger/complex
>> patch. If the DDL replication patch gets committed, it may ditch this
>> part (assuming it happens in the same development cycle).
>>
>> However, my impression was DDL replication would be optional. In which
>> case we still need to handle the transactional case, to support sequence
>> replication without DDL replication enabled.
> 
> As I said before, I don't think this patchset needs to wait for DDL
> replication patch. Let's hope that the later lands in the same release
> and straightens protocol instead of carrying it forever.
> 

OK, I agree with that.


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company