Re: Logical Replication of sequences - Mailing list pgsql-hackers

On Tue, Jun 18, 2024 at 7:30 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Fri, Jun 14, 2024 at 4:04 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Thu, Jun 13, 2024 at 6:14 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > >
> >
> > > >
> > > > Now, say we don't want to maintain the state of sequences for initial
> > > > sync at all then after the error how will we detect if there are any
> > > > pending sequences to be synced? One possibility is that we maintain a
> > > > subscription level flag 'subsequencesync' in 'pg_subscription' to
> > > > indicate whether sequences need sync. This flag would indicate whether
> > > > to sync all the sequences in pg_susbcription_rel. This would mean that
> > > > if there is an error while syncing the sequences we will resync all
> > > > the sequences again. This could be acceptable considering the chances
> > > > of error during sequence sync are low. The benefit is that both the
> > > > REFRESH PUBLICATION SEQUENCES and CREATE SUBSCRIPTION can use the same
> > > > idea and sync all sequences without needing a new relfilenode. Users
> > > > can always refer 'subsequencesync' flag in 'pg_subscription' to see if
> > > > all the sequences are synced after executing the command.
> > >
> > > I think that REFRESH PUBLICATION {SEQUENCES} can be executed even
> > > while the sequence-sync worker is synchronizing sequences. In this
> > > case, the worker might not see new sequences added by the concurrent
> > > REFRESH PUBLICATION {SEQUENCES} command since it's already running.
> > > The worker could end up marking the subsequencesync as completed while
> > > not synchronizing these new sequences.
> > >
> >
> > This is possible but we could avoid REFRESH PUBLICATION {SEQUENCES} by
> > not allowing to change the subsequencestate during the time
> > sequence-worker is syncing the sequences. This could be restrictive
> > but there doesn't seem to be cases where user would like to
> > immediately refresh sequences after creating the subscription.
>
> I'm concerned that users would not be able to add sequences during the
> time the sequence-worker is syncing the sequences. For example,
> suppose we have 10000 sequences and execute REFRESH PUBLICATION
> {SEQUENCES} to synchronize 10000 sequences. Now if we add one sequence
> to the publication and want to synchronize it to the subscriber, we
> have to wait for the current REFRESH PUBLICATION {SEQUENCES} to
> complete, and then execute it again, synchronizing 10001 sequences,
> instead of synchronizing only the new one.
>

I see your point and it could hurt such scenarios even though they
won't be frequent. So, let's focus on our other approach of
maintaining the flag at a per-sequence level in pg_subscription_rel.

> >
> > > >
> > > > > > > Or yet another idea I came up with is that a tablesync worker will
> > > > > > > synchronize both the table and sequences owned by the table. That is,
> > > > > > > after the tablesync worker caught up with the apply worker, the
> > > > > > > tablesync worker synchronizes sequences associated with the target
> > > > > > > table as well. One benefit would be that at the time of initial table
> > > > > > > sync being completed, the table and its sequence data are consistent.
> > > > >
> > > > > Correction; it's not guaranteed that the sequence data and table data
> > > > > are consistent even in this case since the tablesync worker could get
> > > > > on-disk sequence data that might have already been updated.
> > > > >
> > > >
> > > > The benefit of this approach is not clear to me. Our aim is to sync
> > > > all sequences before the upgrade, so not sure if this helps because
> > > > anyway both table values and corresponding sequences can again be
> > > > out-of-sync very quickly.
> > >
> > > Right.
> > >
> > > Given that our aim is to sync all sequences before the upgrade, do we
> > > need to synchronize sequences even at CREATE SUBSCRIPTION time? In
> > > cases where there are a large number of sequences, synchronizing
> > > sequences in addition to tables could be overhead and make less sense,
> > > because sequences can again be out-of-sync quickly and typically
> > > CREATE SUBSCRIPTION is not created just before the upgrade.
> > >
> >
> > I think for the upgrade one should be creating a subscription just
> > before the upgrade. Isn't something similar is done even in the
> > upgrade steps you shared once [1]?
>
> I might be missing something but in the blog post they created
> subscriptions in various ways, waited for the initial table data sync
> to complete, and then set the sequence values with a buffer based on
> the old cluster. What I imagined with this sequence synchronization
> feature is that after the initial table sync completes, we stop to
> execute further transactions on the publisher, synchronize sequences
> using REFRESH PUBLICATION {SEQUENCES}, and resume the application to
> execute transactions on the subscriber. So a subscription would be
> created just before the upgrade, but sequence synchronization would
> not necessarily happen at the same time of the initial table data
> synchronization.
>

It depends on the exact steps of the upgrade. For example, if one
stops the publisher before adding sequences to a subscription either
via create subscription or alter subscription add/set command then
there won't be a need for a separate refresh but OTOH, if one follows
the steps you mentioned then the refresh would be required. As you are
okay, with syncing the sequences while creating a subscription in the
below part of the email, there is not much point in arguing about this
further.

> > Typically users should get all the
> > data from the publisher before the upgrade of the publisher via
> > creating a subscription. Also, it would be better to keep the
> > implementation of sequences close to tables wherever possible. Having
> > said that, I understand your point as well and if you strongly feel
> > that we don't need to sync sequences at the time of CREATE
> > SUBSCRIPTION and others also don't see any problem with it then we can
> > consider that as well.
>
> I see your point that it's better to keep the implementation of
> sequences close to the table one. So I agree that we can start with
> this approach, and we will see how it works in practice and consider
> other options later.
>

makes sense.

> >
> > Marking the state as 'init' when we would have already synced the
> > sequences sounds a bit odd but otherwise, this could also work if we
> > accept that even if the sequences are synced and value could remain in
> > 'init' state (on rollbacks).
>
> I mean that it's just for identifying sequences that need to be
> synced. With the idea of using sequence states in pg_subscription_rel,
> the REFRESH PUBLICATION SEQUENCES command needs to change states to
> something so that the sequence-sync worker can identify which sequence
> needs to be synced. If 'init' sounds odd, we can invent a new state
> for sequences, say 'needs-to-be-syned'.
>

Agreed and I am not sure which is better because there is a value in
keeping the state name the same for both sequences and tables. We
probably need more comments in code and doc updates to make the
behavior clear. We can start with the sequence state as 'init' for
'needs-to-be-sycned' and 'ready' for 'synced' and can change if others
feel so during the review.

--
With Regards,
Amit Kapila.