Re: logical decoding and replication of sequences, take 2

On 7/13/23 16:24, Ashutosh Bapat wrote:
> Thanks for the updated patches. I haven't looked at the patches yet
> but have some responses below.
> 
> On Thu, Jul 13, 2023 at 12:35 AM Tomas Vondra
> <tomas.vondra@enterprisedb.com> wrote:
> 
>>
>>
>> 3) simplify the replicated state
>>
>> As suggested by Ashutosh, it may not be a good idea to replicate the
>> (last_value, log_cnt, is_called) tuple, as that's pretty tightly tied to
>> our internal implementation. Which may not be the right thing for other
>> plugins. So this new patch replicates just "value" which is pretty much
>> (last_value + log_cnt), representing the next value that should be safe
>> to generate on the subscriber (in case of a failover).
>>
> 
> Thanks. That will help.
> 
> 
>> 5) minor tweaks in the built-in replication
>>
>> This adopts the relaxed LOCK code to allow locking sequences during the
>> initial sync, and also adopts the replication of a single value (this
>> affects the "apply" side of that change too).
>>
> 
> I think the problem we are trying to solve with LOCK is not actually
> getting solved. See [2]. Instead your earlier idea of using page LSN
> looks better.
> 

Thanks. I think you may be right, and the interlock may not be
necessary. I've responded to the linked threads, that's probably easier
to follow as it keeps the context.

>>
>> 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?

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.

The new patch detects that, and triggers ERROR on the publisher. And I
think that's the correct thing to do.

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 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.

>> The one thing I'm not really sure about is how it interferes with the
>> replication of DDL. But in principle, if it decodes DDL for ALTER
>> SEQUENCE, I don't see why it would be a problem that we then decode and
>> replicate the WAL for the sequence state. But if it is a problem, we
>> should be able to skip this WAL record with the initial sequence state
>> (which I think should be possible thanks to the "created" flag this
>> patch adds to the WAL record).
> 
> I had suggested a solution in [1] to avoid adding a flag to the WAL
> record. Did you consider it? If you considered it and rejected, I
> would be interested in knowing reasons behind rejecting it. Let me
> repeat here again:
> 
> ```
> We can add a
> decoding routine for RM_SMGR_ID. The decoding routine will add relfilelocator
> in XLOG_SMGR_CREATE record to txn->sequences hash. Rest of the logic will work
> as is. Of course we will add non-sequence relfilelocators as well but that
> should be fine. Creating a new relfilelocator shouldn't be a frequent
> operation. If at all we are worried about that, we can add only the
> relfilenodes associated with sequences to the hash table.
> ```
> 

Thanks for reminding me. In principle I'm not against using the proposed
approach - tracking all relfilenodes created by a transaction, although
I don't think the new flag in xl_seq_rec is a problem, and it's probably
cheaper than having to decode all relfilenode creations.

> 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.

regards

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