Re: logical decoding and replication of sequences, take 2

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(dot)vondra(at)enterprisedb(dot)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.

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

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.

[1] https://www.postgresql.org/message-id/CAExHW5v_vVqkhF4ehST9EzpX1L3bemD1S%2BkTk_-ZVu_ir-nKDw%40mail.gmail.com
[2] https://www.postgresql.org/message-id/CAExHW5vHRgjWzi6zZbgCs97eW9U7xMtzXEQK%2BaepuzoGDsDNtg%40mail.gmail.com
--
Best Wishes,
Ashutosh Bapat