Hi,
I spent a bit of time looking at the proposed change, and unfortunately
logging just the boolean flag does not work. A good example is this bit
from a TAP test added by the patch for built-in replication (which was
not included with the WIP patch):
BEGIN;
ALTER SEQUENCE s RESTART WITH 1000;
SAVEPOINT sp1;
INSERT INTO seq_test SELECT nextval('s') FROM generate_series(1,100);
ROLLBACK TO sp1;
COMMIT;
This is expected to produce:
1131|0|t
but produces
1000|0|f
instead. The reason is very simple - as implemented, the patch simply
checks if the relfilenode is from the same top-level transaction, which
it is, and sets the flag to "true". So we know the sequence changes need
to be queued and replayed as part of this transaction.
But then during decoding, we still queue the changes into the subxact,
which then aborts, and the changes are discarded. That is not how it's
supposed to work, because the new relfilenode is still valid, someone
might do nextval() and commit. And the nextval() may not get WAL-logged,
so we'd lose this.
What I guess we might do is log not just a boolean flag, but the XID of
the subtransaction that created the relfilenode. And then during
decoding we'd queue the changes into this subtransaction ...
0006 in the attached patch series does this, and it seems to fix the TAP
test failure. I left it at the end, to make it easier to run tests
without the patch applied.
There's a couple open questions, though.
- I'm not sure it's a good idea to log XIDs of subxacts into WAL like
this. I think it'd be OK, and there are other records that do that (like
RunningXacts or commit record), but maybe I'm missing something.
- We need the actual XID, not just the SubTransactionId. I wrote
SubTransactionGetXid() to to this, but I did not work with subxacts
this, so it'd be better if someone checked it's dealing with XID and
FullTransactionId correctly.
- I'm a bit concerned how this will perform with deeply nested
subtransactions. SubTransactionGetXid() does pretty much a linear
search, which might be somewhat expensive. And it's a cost put on
everyone who writes WAL, not just the decoding process. Maybe we should
at least limit this to wal_level=logical?
- seq_decode() then uses this XID (for transactional changes) instead of
the XID logged in the record itself. I think that's fine - it's the TXN
where we want to queue the change, after all, right?
- (unrelated) I also noticed that maybe ReorderBufferQueueSequence()
should always expect a valid XID. The code seems to suggest people can
pass InvalidTransactionId in the non-transactional case, but that's not
true because the rb->sequence() then fails.
The attached patches should also fix all the typos reported by Amit
earlier today.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company