Re: logical decoding and replication of sequences, take 2

On Fri, Aug 18, 2023 at 10:37 AM Amit Kapila  wrote:
>
> On Thu, Aug 17, 2023 at 7:13 PM Ashutosh Bapat
>  wrote:
> >
> > On Wed, Aug 16, 2023 at 7:56 PM Tomas Vondra
> >  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.

-- 
With Regards,
Amit Kapila.