Re: [PATCH] Logical decoding support for sequence advances

On 2015-12-14 16:19:33 +0800, Craig Ringer wrote:
> On 14 December 2015 at 11:28, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:
>
> > Hi all
> >
> > Attached is a patch against 9.6 to add support for informing logical
> > decoding plugins of the new sequence last_value when sequence advance WAL
> > records are processed during decoding.
> >
>
>
> Attached a slightly updated version. It just has less spam in the
> regression tests, by adding a new option to test_decoding to show
> sequences, which it doesn't enable except in sequence specific tests.

I think this is quite the wrong approach. You're calling the logical
decoding callback directly from decode.c, circumventing
reorderbuffer.c. While you don't need the actual reordering, you *do*
need the snapshot integration. As you since noticed you can't just
look into the sequence tuple and be happy with that, you need to do
pg_class lookups et al. Furhtermore callbacks can validly expect to have
a snapshot set.

At the very least what you need to do is to SetupHistoricSnapshot(),
then lookup the actual identity of the sequence using
RelidByRelfilenode() and only then you can call the callback.

> Needed to make logical replication based failover possible.

While it'll make it easier, I think it's certainly quite possible to do
so without this feature if you accept some sane restrictions. If you
e.g. define to only handle serial columns (i.e. sequences that
used/owned by exactly one table & column), you can do a 'good enough'
emulation the output plugin.

Take something like BDR's bdr_relcache.c (something which you're going
to end up needing for any nontrivial replication solution). Everytime
you build a cache entry you look up whether there's an owned by
sequence. When decoding an insert or update to that relation, also emit
an 'increase this sequence to at least XXX' message.

While it's not perfect, this has the big advantage of being doable with 9.4.

Greetings,

Andres Freund