From 9be6a6be004f71c80b2f88fd459207866ade061e Mon Sep 17 00:00:00 2001 From: Ashutosh Bapat Date: Tue, 18 Jul 2023 19:24:17 +0530 Subject: [PATCH 7/7] Edits on 0002 patch --- src/backend/replication/logical/decode.c | 2 +- src/backend/replication/logical/logical.c | 8 +-- .../replication/logical/reorderbuffer.c | 56 +++++++++---------- 3 files changed, 32 insertions(+), 34 deletions(-) diff --git a/src/backend/replication/logical/decode.c b/src/backend/replication/logical/decode.c index e08052617d..e24ccd9af6 100644 --- a/src/backend/replication/logical/decode.c +++ b/src/backend/replication/logical/decode.c @@ -1412,7 +1412,7 @@ sequence_decode(LogicalDecodingContext *ctx, XLogRecordBuffer *buf) /* * Should we handle the sequence increment as transactional or not? * - * If the sequence was created in a still-running transaction, treat + * If the sequence was assigned a new relfilenode in the transaction being decoded, treat * it as transactional and queue the increments. Otherwise it needs * to be treated as non-transactional, in which case we send it to * the plugin right away. diff --git a/src/backend/replication/logical/logical.c b/src/backend/replication/logical/logical.c index d2ff674e32..74ba6fd861 100644 --- a/src/backend/replication/logical/logical.c +++ b/src/backend/replication/logical/logical.c @@ -231,10 +231,10 @@ StartupDecodingContext(List *output_plugin_options, /* * To support streaming, we require start/stop/abort/commit/change - * callbacks. The message and truncate callbacks are optional, similar to - * regular output plugins. We however enable streaming when at least one - * of the methods is enabled so that we can easily identify missing - * methods. + * callbacks. The message, sequence and truncate callbacks are optional, + * similar to regular output plugins. We however enable streaming when at + * least one of the methods is enabled so that we can easily identify + * missing methods. * * We decide it here, but only check it later in the wrappers. */ diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c index d0bcc5a2f8..6f5678fd2f 100644 --- a/src/backend/replication/logical/reorderbuffer.c +++ b/src/backend/replication/logical/reorderbuffer.c @@ -77,36 +77,33 @@ * a bit more memory to the oldest subtransactions, because it's likely * they are the source for the next sequence of changes. * - * When decoding sequences, we differentiate between a sequences created - * in a (running) transaction, and sequences created in other (already - * committed) transactions. Changes for sequences created in the same - * top-level transaction are treated as "transactional" i.e. just like - * any other change from that transaction (and discarded in case of a - * rollback). Changes for sequences created earlier are treated as not - * transactional - are processed immediately, as if performed outside - * any transaction (and thus not rolled back). - * + * When decoding sequences, we differentiate between changes to the + * sequences assigned a new relfilenode in in a transaction being decoded, + * and the changes to the sequences created in other (already committed) + * transactions. First type of changes are treated as "transactional" i.e. + * just like any other change from that transaction (and discarded in case + * of a rollback). Second type of changes are treated as non-transactional. + * They are processed immediately, as if performed outside any transaction + * (and thus not rolled back). + * * This mixed behavior is necessary - sequences are non-transactional * (e.g. ROLLBACK does not undo the sequence increments). But for new * sequences, we need to handle them in a transactional way, because if * we ever get some DDL support, the sequence won't exist until the - * transaction gets applied. So we need to ensure the increments don't - * happen until the sequence gets created. - * - * To differentiate which sequences are "old" and which were created - * in a still-running transaction, we track sequences created in running - * transactions in a hash table. Sequences are identified by relfilelocator, - * and we track XID of the (sub)transaction that created it. This means - * that if a transaction does something that changes the relfilelocator - * (like an alter / reset of a sequence), the new relfilelocator will be - * treated as if created in the transaction. The list of sequences gets - * discarded when the transaction completes (commit/rollback). - * - * We don't use the XID to check if it's the same top-level transaction. - * It's enough to know it was created in an in-progress transaction, - * and we know it must be the current one because otherwise it wouldn't - * see the sequence object. + * transaction gets applied. So we need to ensure the changes don't + * happen until the sequence gets created. Similarly when a new relfilenode is assigned to a sequence (because of a DDL) the change does not become visible till the transaction gets committed. It the transaction gets aborted it will never be visible. * + * To differentiate between the changes to the sequences, we track sequences + * which are assigned a new relfilenode in transactions being decoded in a + * hash table. We also track XID of the (sub)transaction that assigned the + * new relfilenode. The list of sequence relfilenodes gets discarded when + * the transaction completes (commit/rollback). + * + * We don't use the XID to check if it's the same top-level transaction. + * It's enough to know it was created in the transaction being decoded, and + * we know it must be the current one because otherwise it wouldn't see the + * sequence object. + * * The XID may be valid even for non-transactional sequences - we simply * keep the XID logged to WAL, it's up to the reorderbuffer to decide if * the increment is transactional. @@ -953,11 +950,12 @@ ReorderBufferQueueMessage(ReorderBuffer *rb, TransactionId xid, } /* - * Treat the sequence increment as transactional? + * Treat the sequence change as transactional? * - * The hash table tracks all sequences created in in-progress transactions, - * so we simply do a lookup (the sequence is identified by relfilende). If - * we find a match, the increment should be handled as transactional. + * The hash table tracks all sequences which are assigned a new relfilenode in + * the transaction being decoded, so we simply do a lookup (the sequence is + * identified by relfilende). If we find a match, the change should be handled + * as transactional. */ bool ReorderBufferSequenceIsTransactional(ReorderBuffer *rb, -- 2.25.1