Re: logical decoding and replication of sequences, take 2
От | Robert Haas |
---|---|
Тема | Re: logical decoding and replication of sequences, take 2 |
Дата | |
Msg-id | CA+TgmoaZrNz5kb7AcSEFRDJy3yT-86qSNnu+qjs3audyEtq2zA@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: logical decoding and replication of sequences, take 2 (Dilip Kumar <dilipbalaut@gmail.com>) |
Ответы |
Re: logical decoding and replication of sequences, take 2
(Dilip Kumar <dilipbalaut@gmail.com>)
|
Список | pgsql-hackers |
On Tue, Feb 20, 2024 at 1:32 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > You might be interested in more detail [1] where I first reported this > problem and also [2] where we concluded why this is not creating a > real problem. > > [1] https://www.postgresql.org/message-id/CAFiTN-vAx-Y%2B19ROKOcWnGf7ix2VOTUebpzteaGw9XQyCAeK6g%40mail.gmail.com > [2] https://www.postgresql.org/message-id/CAFiTN-sYpyUBabxopJysqH3DAp4OZUCTi6m_qtgt8d32vDcWSA%40mail.gmail.com Thanks. Dilip and I just spent a lot of time talking this through on a call. One of the key bits of logic is here: + /* Skip the change if already processed (per the snapshot). */ + if (transactional && + !SnapBuildProcessChange(builder, xid, buf->origptr)) + return; + else if (!transactional && + (SnapBuildCurrentState(builder) != SNAPBUILD_CONSISTENT || + SnapBuildXactNeedsSkip(builder, buf->origptr))) + return; As a stylistic note, I think this would be mode clear if it were written if (transactional) { if (!SnapBuildProcessChange()) return; } else { if (something else) return; }. Now, on to correctness. It's possible for us to identify a transactional change as non-transactional if smgr_decode() was called for the relfilenode before SNAPBUILD_FULL_SNAPSHOT was reached. In that case, if !SnapBuildProcessChange() would have been true, then we need SnapBuildCurrentState(builder) != SNAPBUILD_CONSISTENT || SnapBuildXactNeedsSkip(builder, buf->origptr) to also be true. Otherwise, we'll process this change when we wouldn't have otherwise. But Dilip made an argument to me about this which seems correct to me. snapbuild.h says that SNAPBUILD_CONSISTENT is reached only when we find a point where any transaction that was running at the time we reached SNAPBUILD_FULL_SNAPSHOT have finished. So if this transaction is one for which we incorrectly identified the sequence change as non-transactional, then we cannot be in the SNAPBUILD_CONSISTENT state yet, so SnapBuildCurrentState(builder) != SNAPBUILD_CONSISTENT will be true and hence whole "or" condition we'll be true and we'll return. So far, so good. I think, anyway. I haven't comprehensively verified that the comment in snapbuild.h accurately reflects what the code actually does. But if it does, then presumably we shouldn't see a record for which we might have mistakenly identified a change as non-transactional after reaching SNAPBUILD_CONSISTENT, which seems to be good enough to guarantee that the mistake won't matter. However, the logic in smgr_decode() doesn't only care about the snapshot state. It also cares about the fast-forward flag: + if (SnapBuildCurrentState(builder) < SNAPBUILD_FULL_SNAPSHOT || + ctx->fast_forward) + return; Let's say fast_forward is true. Then smgr_decode() is going to skip recording anything about the relfilenode, so we'll identify all sequence changes as non-transactional. But look at how this case is handled in seq_decode(): + if (ctx->fast_forward) + { + /* + * We need to set processing_required flag to notify the sequence + * change existence to the caller. Usually, the flag is set when + * either the COMMIT or ABORT records are decoded, but this must be + * turned on here because the non-transactional logical message is + * decoded without waiting for these records. + */ + if (!transactional) + ctx->processing_required = true; + + return; + } This seems suspicious. Why are we testing the transactional flag here if it's guaranteed to be false? My guess is that the person who wrote this code thought that the flag would be accurate even in this case, but that doesn't seem to be true. So this case probably needs some more thought. It's definitely not great that this logic is so complicated; it's really hard to verify that all the tests match up well enough to keep us out of trouble. -- Robert Haas EDB: http://www.enterprisedb.com
В списке pgsql-hackers по дате отправления:
Предыдущее
От: Quan ZongliangДата:
Сообщение: Change the bool member of the Query structure to bits
Следующее
От: Daniel GustafssonДата:
Сообщение: Re: Replace current implementations in crypt() and gen_salt() to OpenSSL