On Wed, Feb 21, 2024 at 2:43 PM Dilip Kumar <dilipbal...@gmail.com> wrote: > So the problem is that we might consider the transaction change as > non-transaction and mark this flag as true.
But it's not "might" right? It's absolutely 100% certain that we will consider that transaction's changes as non-transactional ... because when we're in fast-forward mode, the table of new relfilenodes is not built, and so whenever we check whether any transaction made a new relfilenode for this sequence, the answer will be no. > But what would have > happened if we would have identified it correctly as transactional? > In such cases, we wouldn't have set this flag here but then we would > have set this while processing the DecodeAbort/DecodeCommit, so the > net effect would be the same no? You may question what if the > Abort/Commit WAL never appears in the WAL, but this flag is > specifically for the upgrade case, and in that case we have to do a > clean shutdown so may not be an issue. But in the future, if we try > to use 'ctx->processing_required' for something else where the clean > shutdown is not guaranteed then this flag can be set incorrectly. > > I am not arguing that this is a perfect design but I am just making a > point about why it would work. Even if this argument is correct (and I don't know if it is), the code and comments need some updating. We should not be testing a flag that is guaranteed false with comments that make it sound like the value of the flag is trustworthy when it isn't. -- Robert Haas EDB: http://www.enterprisedb.com