Horton

Re: logical decoding and replication of sequences

Amit Kapila <amit.kapila16@gmail.com>

View thread View on postgr.es
From: Amit Kapila <amit.kapila16@gmail.com>
To: Tomas Vondra <tomas.vondra@enterprisedb.com>
Cc: Petr Jelinek <petr.jelinek@enterprisedb.com>, Peter Eisentraut <peter.eisentraut@enterprisedb.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Date: 2022-03-26T07:28:15Z
Lists: pgsql-hackers 
On Fri, Mar 25, 2022 at 10:20 PM Tomas Vondra
<tomas.vondra@enterprisedb.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).

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

> For a while I was thinking that maybe this means we don't need the
> transactional behavior at all, but I think we do - we have to handle
> ALTER SEQUENCE cases that are transactional.
>

I need some time to think about this. At all places, it is mentioned
as creating a sequence for transactional cases which at the very least
need some tweak.

-- 
With Regards,
Amit Kapila.

Commits

  1. Fix cache invalidation bug in recovery_prefetch.

  2. Revert "Logical decoding of sequences"

  3. Minor improvements in sequence decoding code and docs

  4. Handle sequences in preprocess_pubobj_list

  5. Update tab-completion for CREATE PUBLICATION with sequences

  6. Add decoding of sequences to built-in replication

  7. Stabilize test_decoding touching with sequences

  8. Call ReorderBufferProcessXid from sequence_decode

  9. Add decoding of sequences to test_decoding

  10. Replace Test::More plans with done_testing

  11. Logical decoding of sequences

  12. Respect permissions within logical replication.