On Wed, Feb 21, 2024 at 2:43 PM Dilip Kumar <dilipbalaut@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