On Sun, Dec 12, 2021 at 3:09 AM Dilip Kumar <dilipbal...@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