Re: logical decoding and replication of sequences

Hi,

Here's a an updated version of this patch - rebased to current master,
and fixing some of the issues raised in Peter's review.

Mainly, this adds a TAP test to src/test/subscription, focusing on
testing the various situations with transactional and non-transactional
behavior (with subtransactions and various ROLLBACK versions).

This new TAP test however uncovered an issue with wait_for_catchup(),
because that uses pg_current_wal_lsn() to wait for replication of all
the changes. But that does not work when the sequence gets advanced in a
transaction that is then aborted, e.g. like this:

BEGIN;
SELECT nextval('s') FROM generate_series(1,100);
ROLLBACK;

The root cause is that pg_current_wal_lsn() uses the LogwrtResult.Write,
which is updated by XLogFlush() - but only in RecordTransactionCommit.
Which makes sense, because only the committed stuff is "visible". But
the non-transactional behavior changes this, because now some of the
changes from aborted transactions may need to be replicated. Which means
the wait_for_catchup() ends up not waiting for the sequence change.

One option would be adding XLogFlush() to RecordTransactionAbort(), but
my guess is we don't do that intentionally (even though aborts should be
fairly rare in most workloads).

I'm not entirely sure changing this (replicating changes from aborted
xacts) is a good idea. Maybe it'd be better to replicate this "lazily" -
instead of replicating the advances right away, we might remember which
sequences were advanced in the transaction, and then replicate current
state for those sequences at commit time.

The idea is that if an increment is "invisible" we probably don't need
to replicate it, it's fine to replicate the next "visible" increment. So
for example given

BEGIN;
SELECT nextval('s');
ROLLBACK;

BEGIN;
SELECT nextval('s');
COMMIT;

we don't need to replicate the first change, but we need to replicate
the second one.

The trouble is we don't decode individual sequence advances, just those
that update the sequence tuple (so every 32 values or so). So we'd need
to remeber the first increment, in a way that is (a) persistent across
restarts and (b) shared by all backends.

The other challenge seems to be ordering of the changes - at the moment
we have no issues with this, because increments on the same sequence are
replicated immediately, in the WAL order. But postponing them to commit
time would affect this order.

I've also briefly looked at the code duplication - there's a couple
functions in the patch that I shamelessly copy-pasted and tweaked to
handle sequences instead of tables:

publicationcmds.c
-----------------

1) OpenTableList/CloseTableList - > OpenSequenceList/CloseSequenceList

Trivial differences, trivial to get rid of - the only difference is
pretty much just table_open vs. relation open.

2) AlterPublicationTables -> AlterPublicationSequences

This is a bit more complicated, because the tables also handle
inheritance (which I think does not apply to sequences). Other than
that, it's calling the functions from (1).

subscriptioncmds.c
------------------

1) fetch_table_list, fetch_sequence_list

Minimal differences, essentially just the catalog name.

2) AlterSubscription_refresh

A lot of duplication, but the code handling tables and sequences is
almost exactly the same and can be reused fairly easily (moved to a
separate function, called for tables and then sequences).

regards

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