Re: logical decoding and replication of sequences

On 4/2/22 12:35, Amit Kapila wrote:
> On Fri, Apr 1, 2022 at 8:32 PM Tomas Vondra
> <tomas(dot)vondra(at)enterprisedb(dot)com> wrote:
>>
>> On 3/28/22 07:29, Amit Kapila wrote:
>>> I thought about changing snapshot dealing of
>>> non-transactional sequence changes similar to transactional ones but
>>> that also won't work because it is only at commit we decide whether we
>>> can send the changes.
>>>
>> I wonder if there's some earlier LSN (similar to the consistent point)
>> which might be useful for this.
>>
>> Or maybe we should queue even the non-transactional changes, not
>> per-transaction but in a global list, and then at each commit either
>> discard inspect them (at that point we know the lowest LSN for all
>> transactions and the consistent point). Seems complex, though.
>>
>
> I couldn't follow '..discard inspect them ..'. Do you mean we inspect
> them and discard whichever are not required? It seems here we are
> talking about a new global ReorderBufferGlobal instead of
> ReorderBufferTXN to collect these changes but we don't need only
> consistent point LSN because we do send if the commit of containing
> transaction is after consistent point LSN, so we need some transaction
> information as well. I think it could bring new challenges.
>

Sorry for the gibberish. Yes, I meant to discard sequence changes that
are no longer needed, due to being "obsoleted" by the applied change. We
must not apply "older" changes (using LSN) because that would make the
sequence go backwards.

I'm not entirely sure whether the list of changes should be kept in TXN
or in the global reorderbuffer object - we need to track which TXN the
change belongs to (because of transactional changes) but we also need to
discard the unnecessary changes efficiently (and walking TXN might be
expensive).

But yes, I'm sure there will be challenges. One being that tracking just
the decoded WAL stuff is not enough, because nextval() may not generate
WAL. But we still need to make sure the increment is replicated.

What I think we might do is this:

- add a global list of decoded sequence increments to ReorderBuffer

- at each commit/abort walk the list, walk the list and consider all
increments up to the commit LSN that "match" (non-transactional match
all TXNs, transactional match only the current TXN)

- replicate the last "matching" status for each sequence, discard the
processed ones

We could probably optimize this by not tracking every single increment,
but merge them "per transaction", I think.

I'm sure this description is pretty rough and will need refining, handle
various corner-cases etc.

>>> For the transactional case, as we are considering the create sequence
>>> operation as transactional, we would unnecessarily queue them even
>>> though that is not required. Basically, they don't need to be
>>> considered transactional and we can simply ignore such messages like
>>> other DDLs. But for that probably we need to distinguish Alter/Create
>>> case which may or may not be straightforward. Now, queuing them is
>>> probably harmless unless it causes the transaction to spill/stream.
>>>
>>
>> I'm not sure I follow. Why would we queue them unnecessarily?
>>
>> Also, there's the bug with decoding changes in transactions that create
>> the sequence and add it to a publication. I think the agreement was that
>> this behavior was incorrect, we should not decode changes until the
>> subscription is refreshed. Doesn't that mean can't be any CREATE case,
>> just ALTER?
>>
>
> Yeah, but how will we distinguish them. Aren't they using the same
> kind of WAL record?
>

Same WAL record, but the "created" flag which should distinguish these
two cases, IIRC.

regards

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