Re: logical decoding and replication of sequences

On Sat, Mar 26, 2022 at 6:56 PM Tomas Vondra
<tomas(dot)vondra(at)enterprisedb(dot)com> wrote:
>
>
>
> On 3/26/22 08:28, Amit Kapila wrote:
> > On Fri, Mar 25, 2022 at 10:20 PM Tomas Vondra
> > <tomas(dot)vondra(at)enterprisedb(dot)com> wrote:
> >>
> >> Hmm, so fixing this might be a bit trickier than I expected.
> >>
> >> Firstly, currently we only send nspname/relname in the sequence message,
> >> not the remote OID or schema. The idea was that for sequences we don't
> >> really need schema info, so this seemed OK.
> >>
> >> But should_apply_changes_for_rel() needs LogicalRepRelMapEntry, and to
> >> create/maintain that those records we need to send the schema.
> >>
> >> Attached is a WIP patch does that.
> >>
> >> Two places need more work, I think:
> >>
> >> 1) maybe_send_schema needs ReorderBufferChange, but we don't have that
> >> for sequences, we only have TXN. I created a simple wrapper, but maybe
> >> we should just tweak maybe_send_schema to use TXN.
> >>
> >> 2) The transaction handling in is a bit confusing. The non-transactional
> >> increments won't have any explicit commit later, so we can't just rely
> >> on begin_replication_step/end_replication_step. But I want to try
> >> spending a bit more time on this.
> >>
> >
> > I didn't understand what you want to say in point (2).
> >
>
> My point is that handle_apply_sequence() either needs to use the same
> transaction handling as other apply methods, or start (and commit) a
> separate transaction for the "transactional" case.
>
> Which means we can't use the begin_replication_step/end_replication_step
> and the current code seems a bit complex. And I'm not sure it's quite
> correct. So this place needs more work.
>
> >>
> >> But there's a more serious issue, I think. So far, we allowed this:
> >>
> >> BEGIN;
> >> CREATE SEQUENCE s2;
> >> ALTER PUBLICATION p ADD SEQUENCE s2;
> >> INSERT INTO seq_test SELECT nextval('s2') FROM generate_series(1,100);
> >> COMMIT;
> >>
> >> and the behavior was that we replicated the changes. But with the patch
> >> applied, that no longer happens, because should_apply_changes_for_rel
> >> says the change should not be applied.
> >>
> >> And after thinking about this, I think that's correct - we can't apply
> >> changes until ALTER SUBSCRIPTION ... REFRESH PUBLICATION gets executed,
> >> and we can't do that until the transaction commits.
> >>
> >> So I guess that's correct, and the current behavior is a bug.
> >>
> >
> > Yes, I also think that is a bug.
> >
>
> OK

I also think that this is a bug. Given this behavior is a bug and
newly-added sequence data should be replicated only after ALTER
SUBSCRIPTION ... REFRESH PUBLICATION, is there any case where the
sequence message applied on the subscriber is transactional?

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/