Re: logical decoding and replication of sequences, take 2 - Mailing list pgsql-hackers
From | Ashutosh Bapat |
---|---|
Subject | Re: logical decoding and replication of sequences, take 2 |
Date | |
Msg-id | CAExHW5taBfSbE679mypZEK=eNhHS-_TUNTSmfRhd74XNamUF7w@mail.gmail.com Whole thread Raw |
In response to | Re: logical decoding and replication of sequences, take 2 (Amit Kapila <amit.kapila16@gmail.com>) |
Responses |
Re: logical decoding and replication of sequences, take 2
|
List | pgsql-hackers |
On Fri, Aug 18, 2023 at 4:28 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Fri, Aug 18, 2023 at 10:37 AM Amit Kapila <amit.kapila16@gmail.com> wrote: > > > > On Thu, Aug 17, 2023 at 7:13 PM Ashutosh Bapat > > <ashutosh.bapat.oss@gmail.com> wrote: > > > > > > On Wed, Aug 16, 2023 at 7:56 PM Tomas Vondra > > > <tomas.vondra@enterprisedb.com> wrote: > > > > > > > > > > > > > > But whether or not that's the case, downstream should not request (and > > > > > hence receive) any changes that have been already applied (and > > > > > committed) downstream as a principle. I think a way to achieve this is > > > > > to update the replorigin_session_origin_lsn so that a sequence change > > > > > applied once is not requested (and hence sent) again. > > > > > > > > > > > > > I guess we could update the origin, per attached 0004. We don't have > > > > timestamp to set replorigin_session_origin_timestamp, but it seems we > > > > don't need that. > > > > > > > > The attached patch merges the earlier improvements, except for the part > > > > that experimented with adding a "fake" transaction (which turned out to > > > > have a number of difficult issues). > > > > > > 0004 looks good to me. > > > > > > + { > > CommitTransactionCommand(); > > + > > + /* > > + * Update origin state so we don't try applying this sequence > > + * change in case of crash. > > + * > > + * XXX We don't have replorigin_session_origin_timestamp, but we > > + * can just leave that set to 0. > > + */ > > + replorigin_session_origin_lsn = seq.lsn; > > > > IIUC, your proposal is to update the replorigin_session_origin_lsn, so > > that after restart, it doesn't use some prior origin LSN to start with > > which can in turn lead the sequence to go backward. If so, it should > > be updated before calling CommitTransactionCommand() as we are doing > > in apply_handle_commit_internal(). If that is not the intention then > > it is not clear to me how updating replorigin_session_origin_lsn after > > commit is helpful. > > > > typedef struct ReplicationState > { > ... > /* > * Location of the latest commit from the remote side. > */ > XLogRecPtr remote_lsn; > > This is the variable that will be updated with the value of > replorigin_session_origin_lsn. This means we will now track some > arbitrary LSN location of the remote side in this variable. The above > comment makes me wonder if there is anything we are missing or if it > is just a matter of updating this comment because before the patch we > always adhere to what is written in the comment. I don't think we are missing anything. This value is used to track the remote LSN upto which all the commits from upstream have been applied locally. Since a non-transactional sequence change is like a single WAL record transaction, it's LSN acts as the LSN of the mini-commit. So it should be fine to update remote_lsn with sequence WAL record's end LSN. That's what the patches do. I don't see any hazard. But you are right, we need to update comments. Here and also at other places like replorigin_session_advance() which uses remote_commit as name of the argument which gets assigned to ReplicationState::remote_lsn. -- Best Wishes, Ashutosh Bapat
pgsql-hackers by date: