Re: logical decoding and replication of sequences

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

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

Understood.

> At all places, it is mentioned
> as creating a sequence for transactional cases which at the very least
> need some tweak.
>

Which places?

regards

--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company