Re: logical decoding and replication of sequences

Hi,

On 9/23/21 12:27 PM, Peter Eisentraut wrote:
> On 30.07.21 20:26, Tomas Vondra wrote:
>> Here's a an updated version of this patch - rebased to current master,
>> and fixing some of the issues raised in Peter's review.
>
> This patch needs an update, as various conflicts have arisen now.
>
> As was discussed before, it might be better to present a separate patch
> for just the logical decoding part for now, since the replication and
> DDL stuff has the potential to conflict heavily with other patches being
> discussed right now.  It looks like cutting this patch in two should be
> doable easily.
>

Attached is the rebased patch, split into three parts:

1) basic decoding infrastructure (decoding, reorderbuffer etc.)
2) support for sequences in test_decoding
3) support for sequences in built-in replication (catalogs, syntax, ...)

The last part is the largest one - I'm sure we'll have discussions about
the grammar, adding sequences automatically, etc. But as you said, let's
focus on the first part, which deals with the required decoding stuff.

I've added a couple comments, explaining how we track sequences, why we
need the XID in nextval() etc. I've also added streaming support.

> I looked through the test cases in test_decoding again.  It all looks
> pretty sensible.  If anyone can think of any other tricky or dubious
> cases, we can add them there.  It's easiest to discuss these things with
> concrete test cases rather than in theory.
>
> One slightly curious issue is that this can make sequence values go
> backwards, when seen by the logical decoding consumer, like in the test
> case:
>
> + BEGIN
> + sequence: public.test_sequence transactional: 1 created: 1 last_value:
> 1, log_cnt: 0 is_called: 0
> + COMMIT
> + sequence: public.test_sequence transactional: 0 created: 0 last_value:
> 33, log_cnt: 0 is_called: 1
> + BEGIN
> + sequence: public.test_sequence transactional: 1 created: 1 last_value:
> 4, log_cnt: 0 is_called: 1
> + COMMIT
> + sequence: public.test_sequence transactional: 0 created: 0 last_value:
> 334, log_cnt: 0 is_called: 1
>
> I suppose that's okay, since it's not really the intention that someone
> is concurrently consuming sequence values on the subscriber.  Maybe
> something for the future.  Fixing that would require changing the way
> transactional sequence DDL updates these values, so it's not directly
> the job of the decoding to address this.
>

Yeah, that's due to how ALTER SEQUENCE does things, and I agree redoing
that seems well out of scope for this patch. What seems a bit suspicious
is that some of the ALTER SEQUENCE changes have "created: 1" - it's
probably correct, though, because those ALTER SEQUENCE statements can be
rolled-back, so we see it as if a new sequence is created. The flag name
might be a bit confusing, though.

> A small thing I found: Maybe the text that test_decoding produces for
> sequences can be made to look more consistent with the one for tables.
> For example, in
>
> + BEGIN
> + sequence: public.test_table_a_seq transactional: 1 created: 1
> last_value: 1, log_cnt: 0 is_called: 0
> + sequence: public.test_table_a_seq transactional: 1 created: 0
> last_value: 33, log_cnt: 0 is_called: 1
> + table public.test_table: INSERT: a[integer]:1 b[integer]:100
> + table public.test_table: INSERT: a[integer]:2 b[integer]:200
> + COMMIT
>
> note how the punctuation is different.

I did tweak this a bit, hopefully it's more consistent.

regards

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