On Sun, Dec 12, 2021 at 3:09 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> Correct, I have done this cleanup, apart from this we have dropped the
> fsyc request in create database failure case as well and also need to
> drop buffer in error case of creatdb as well as movedb. I have also
> fixed the other issue for which you gave the patch (a bit differently)
> basically, in case of movedb the source and destination dboid are same
> so we don't need an additional parameter and also readjusted the
> conditions to avoid nested if.
Amazingly to me given how much time has passed, these patches still
apply, although I think there are a few outstanding issues that you
promised to fix in the next version and haven't yet addressed.
In 0007, I think you will need to work a bit harder. I don't think
that you can just add a second argument to
ForgetDatabaseSyncRequests() that makes it do something other than
what the name of the function suggests but without renaming the
function or updating any comments. Elsewhere we have things like
TablespaceCreateDbspace and ResetUnloggedRelationsInDbspaceDir so
perhaps we ought to just add a new function with a name inspired by
those precedents alongside the existing one, rather than doing it this
way.
In 0008, this is a bit confusing:
+ PageInit(dstPage, BufferGetPageSize(dstBuf), 0);
+ memcpy(dstPage, srcPage, BLCKSZ);
After a minute, I figured out that the point here was to force
log_newpage() to actually set the LSN, but how about a comment?
I kind of wonder whether GetDatabaseRelationList should be broken into
two functions so that don't have quite such deep nesting. And I wonder
if maybe the return value of GetActiveSnapshot() should be cached in a
local variable.
On the whole I think there aren't huge code-level issues here, even if
things need to be tweaked here and there and bugs fixed. The real key
is arriving at a set of design trade-offs that doesn't make anyone too
upset.
--
Robert Haas
EDB: http://www.enterprisedb.com
