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