Re: [HACKERS] logical decoding of two-phase transactions
От | Peter Smith |
---|---|
Тема | Re: [HACKERS] logical decoding of two-phase transactions |
Дата | |
Msg-id | CAHut+Pt6zB-YffCrMo7+ZOKn7C2yXkNYnuQTdbStEJJJXZZXaw@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: [HACKERS] logical decoding of two-phase transactions (Ajin Cherian <itsajin@gmail.com>) |
Список | pgsql-hackers |
FYI - Please find attached code coverage reports which I generated (based on the v12 patches) after running the following tests: 1. cd contrib/test_decoding; make check 2. cd src/test/subscriber; make check Kind Regards, Peter Smith. Fujitsu Australia On Tue, Oct 27, 2020 at 8:55 PM Ajin Cherian <itsajin@gmail.com> wrote: > > On Mon, Oct 26, 2020 at 6:49 PM Peter Smith <smithpb2250@gmail.com> wrote: > > > > Hi Ajin. > > > > I checked the to see how my previous review comments (of v10) were > > addressed by the latest patches (currently v12) > > > > There are a couple of remaining items. > > > > --- > > > > ==================== > > v12-0001. File: doc/src/sgml/logicaldecoding.sgml > > ==================== > > > > COMMENT > > Section 49.6.1 > > Says: > > An output plugin may also define functions to support streaming of > > large, in-progress transactions. The stream_start_cb, stream_stop_cb, > > stream_abort_cb, stream_commit_cb, stream_change_cb, and > > stream_prepare_cb are required, while stream_message_cb and > > stream_truncate_cb are optional. > > > > An output plugin may also define functions to support two-phase > > commits, which are decoded on PREPARE TRANSACTION. The prepare_cb, > > commit_prepared_cb and rollback_prepared_cb callbacks are required, > > while filter_prepare_cb is optional. > > ~ > > I was not sure how the paragraphs are organised. e.g. 1st seems to be > > about streams and 2nd seems to be about two-phase commit. But they are > > not mutually exclusive, so I guess I thought it was odd that > > stream_prepare_cb was not mentioned in the 2nd paragraph. > > > > Or maybe it is OK as-is? > > > > I've added stream_prepare_cb to the 2nd paragraph as well. > > > > ==================== > > v12-0002. File: contrib/test_decoding/expected/two_phase.out > > ==================== > > > > COMMENT > > Line 26 > > PREPARE TRANSACTION 'test_prepared#1'; > > -- > > SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, > > NULL, 'two-phase-commit', '1', 'include-xids', '0', > > 'skip-empty-xacts', '1'); > > ~ > > Seems like a missing comment to explain the expectation of that select. > > > > --- > > > > Updated. > > > COMMENT > > Line 80 > > -- The insert should show the newly altered column. > > ~ > > Do you also need to mention something about the DDL not being present > > in the decoding? > > > > Updated. > > > ==================== > > v12-0002. File: src/backend/replication/logical/reorderbuffer.c > > ==================== > > > > COMMENT > > Line 1807 > > /* Here we are streaming and part of the PREPARE of a two-phase commit > > * The full cleanup will happen as part of the COMMIT PREPAREDs, so now > > * just truncate txn by removing changes and tuple_cids > > */ > > ~ > > Something seems strange about the first sentence of that comment > > > > --- > > > > COMMENT > > Line 1944 > > /* Discard the changes that we just streamed. > > * This can only be called if streaming and not part of a PREPARE in > > * a two-phase commit, so set prepared flag as false. > > */ > > ~ > > I thought since this comment that is asserting various things, that > > should also actually be written as code Assert. > > > > --- > > Added an assert. > > > > > COMMENT > > Line 2401 > > /* > > * We are here due to one of the 3 scenarios: > > * 1. As part of streaming in-progress transactions > > * 2. Prepare of a two-phase commit > > * 3. Commit of a transaction. > > * > > * If we are streaming the in-progress transaction then discard the > > * changes that we just streamed, and mark the transactions as > > * streamed (if they contained changes), set prepared flag as false. > > * If part of a prepare of a two-phase commit set the prepared flag > > * as true so that we can discard changes and cleanup tuplecids. > > * Otherwise, remove all the > > * changes and deallocate the ReorderBufferTXN. > > */ > > ~ > > The above comment is beyond my understanding. Anything you could do to > > simplify it would be good. > > > > For example, when viewing this function in isolation I have never > > understood why the streaming flag and rbtxn_prepared(txn) flag are not > > possible to be set at the same time? > > > > Perhaps the code is relying on just internal knowledge of how this > > helper function gets called? And if it is just that, then IMO there > > really should be some Asserts in the code to give more assurance about > > that. (Or maybe use completely different flags to represent those 3 > > scenarios instead of bending the meanings of the existing flags) > > > > Left this for now, probably re-look at this at a later review. > But just to explain; this function is what does the main decoding of > changes of a transaction. > At what point this decoding happens is what this feature and the > streaming in-progress feature is about. As of PG13, this decoding only > happens at commit time. With the streaming of in-progress txn feature, > this began to happen (if streaming enabled) at the time when the > memory limit for decoding transactions was crossed. This 2PC feature > is supporting decoding at the time of a PREPARE transaction. > Now, if streaming is enabled and streaming has started as a result of > crossing the memory threshold, then there is no need to > again begin streaming at a PREPARE transaction as the transaction that > is being prepared has already been streamed. Which is why this > function will not be called when a streaming transaction is prepared > as part of a two-phase commit. > > > ==================== > > v12-0003. File: src/backend/access/transam/twophase.c > > ==================== > > > > COMMENT > > Line 557 > > @@ -548,6 +548,33 @@ MarkAsPrepared(GlobalTransaction gxact, bool lock_held) > > } > > > > /* > > + * LookupGXact > > + * Check if the prepared transaction with the given GID is around > > + */ > > +bool > > +LookupGXact(const char *gid) > > +{ > > + int i; > > + bool found = false; > > ~ > > Alignment of the variable declarations in LookupGXact function > > > > --- > > Updated. > > Amit, I have also updated your comment about removing function > declaration from commit 1 and I've added it to commit 2. Also removed > whitespace errors. > > regards, > Ajin Cherian > Fujitsu Australia
Вложения
В списке pgsql-hackers по дате отправления:
Следующее
От: Tom LaneДата:
Сообщение: Re: Patch to fix FK-related selectivity estimates with constants