Re: Skip collecting decoded changes of already-aborted transactions
От | Peter Smith |
---|---|
Тема | Re: Skip collecting decoded changes of already-aborted transactions |
Дата | |
Msg-id | CAHut+Pu+5PLiA57rgepPaMPc5LpEDd9JULk2TkmWh32SHuxbQw@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: Skip collecting decoded changes of already-aborted transactions (Masahiko Sawada <sawada.mshk@gmail.com>) |
Список | pgsql-hackers |
Hi, here are some review comments for your patch v4-0001. ====== contrib/test_decoding/sql/stats.sql 1. Huh? The test fails because the "expected results" file for these new tests is missing from the patch. ====== .../replication/logical/reorderbuffer.c 2. static void ReorderBufferTruncateTXN(ReorderBuffer *rb, ReorderBufferTXN *txn, - bool txn_prepared); + bool txn_prepared, bool mark_streamed); IIUC this new 'mark_streamed' parameter is more like a prerequisite for the other conditions to decide to mark the tx as streamed -- i.e. it is more like 'can_mark_streamed', so I felt the name should be changed to be like that (everywhere it is used). ~~~ 3. ReorderBufferTruncateTXN - * 'txn_prepared' indicates that we have decoded the transaction at prepare - * time. + * If mark_streamed is true, we could mark the transaction as streamed. + * + * 'streaming_txn' indicates that the given transaction is a streaming transaction. */ static void -ReorderBufferTruncateTXN(ReorderBuffer *rb, ReorderBufferTXN *txn, bool txn_prepared) +ReorderBufferTruncateTXN(ReorderBuffer *rb, ReorderBufferTXN *txn, bool txn_prepared, + bool mark_streamed) ~ What's that new comment about 'streaming_txn' for? It seemed unrelated to the patch code. ~~~ 4. /* * Mark the transaction as streamed. * * The top-level transaction, is marked as streamed always, even if it * does not contain any changes (that is, when all the changes are in * subtransactions). * * For subtransactions, we only mark them as streamed when there are * changes in them. * * We do it this way because of aborts - we don't want to send aborts for * XIDs the downstream is not aware of. And of course, it always knows * about the toplevel xact (we send the XID in all messages), but we never * stream XIDs of empty subxacts. */ if (mark_streamed && (!txn_prepared) && (rbtxn_is_toptxn(txn) || (txn->nentries_mem != 0))) txn->txn_flags |= RBTXN_IS_STREAMED; ~~ With the patch introduction of the new parameter, I felt this code might be better if it was refactored as follows: /* Mark the transaction as streamed, if appropriate. */ if (can_mark_streamed) { /* ... large comment */ if ((!txn_prepared) && (rbtxn_is_toptxn(txn) || (txn->nentries_mem != 0))) txn->txn_flags |= RBTXN_IS_STREAMED; } ~~~ 5. ReorderBufferPrepare - if (txn->concurrent_abort && !rbtxn_is_streamed(txn)) + if (!txn_aborted && rbtxn_did_abort(txn) && !rbtxn_is_streamed(txn)) rb->prepare(rb, txn, txn->final_lsn); ~ Maybe I misunderstood this logic, but won't a "concurrent abort" cause your new Assert added in ReorderBufferProcessTXN to fail? + /* Update transaction status */ + Assert((curtxn->txn_flags & (RBTXN_COMMITTED | RBTXN_ABORTED)) == 0); ~~~ 6. ReorderBufferCheckTXNAbort + /* Check the transaction status using CLOG lookup */ + if (TransactionIdIsInProgress(txn->xid)) + return false; + + if (TransactionIdDidCommit(txn->xid)) + { + /* + * Remember the transaction is committed so that we can skip CLOG + * check next time, avoiding the pressure on CLOG lookup. + */ + txn->txn_flags |= RBTXN_COMMITTED; + return false; + } IIUC the purpose of the TransactionIdDidCommit() was to avoid the overhead of calling the TransactionIdIsInProgress(). So, shouldn't the order of these checks be swapped? Otherwise, there might be 1 extra unnecessary call to TransactionIdIsInProgress() next time. ====== src/include/replication/reorderbuffer.h 7. #define RBTXN_PREPARE 0x0040 #define RBTXN_SKIPPED_PREPARE 0x0080 #define RBTXN_HAS_STREAMABLE_CHANGE 0x0100 +#define RBTXN_COMMITTED 0x0200 +#define RBTXN_ABORTED 0x0400 For consistency with the existing bitmask names, I guess these should be named: - RBTXN_COMMITTED --> RBTXN_IS_COMMITTED - RBTXN_ABORTED --> RBTXN_IS_ABORTED ~~~ 8. Similarly, IMO the macros should have the same names as the bitmasks, like the other nearby ones generally seem to. rbtxn_did_commit --> rbtxn_is_committed rbtxn_did_abort --> rbtxn_is_aborted ====== 9. Also, attached is a top-up patch for other cosmetic nitpicks: - comment wording - typos in comments - excessive or missing blank lines - etc. ====== Kind Regards, Peter Smith. Fujitsu Australia
Вложения
В списке pgsql-hackers по дате отправления:
Предыдущее
От: "Erica Zhang"Дата:
Сообщение: Re:Re: Re: Add support to TLS 1.3 cipher suites and curves lists
Следующее
От: Amit KapilaДата:
Сообщение: Re: confirmed flush lsn seems to be move backward in certain error cases