Обсуждение: Writing WAL for relcache invalidation: pg_internal.init
Enclose a patch for new WAL records for relcache invalidation. Not conclusively tested, but seems to work fine, so please regard this as a prototype-needs-review. We definitely need a regr test framework to support this type of patch. -- Simon Riggs EnterpriseDB http://www.enterprisedb.com
Вложения
"Simon Riggs" <simon@2ndquadrant.com> writes: > Enclose a patch for new WAL records for relcache invalidation. I don't think this works. RelationCacheInitFileInvalidate is executed post-commit, which means that there's a window between commit and where you propose to write the WAL entry. A crash and restart in that interval would leave the catalog changes committed, but not reflected into pg_internal.init. I think we're probably better off to just forcibly remove the init file during post-recovery cleanup. The easiest place to do this might be BuildFlatFiles, which has to scan pg_database anyway ... regards, tom lane
On Wed, 2006-11-01 at 12:05 -0500, Tom Lane wrote: > I think we're probably better off to just forcibly remove the init file > during post-recovery cleanup. The easiest place to do this might be > BuildFlatFiles, which has to scan pg_database anyway ... Presumably not rebuilding it, since we can let that occur naturally. I'll submit a patch tomorrow. -- Simon Riggs EnterpriseDB http://www.enterprisedb.com
On Wed, 2006-11-01 at 12:05 -0500, Tom Lane wrote: > "Simon Riggs" <simon@2ndquadrant.com> writes: > > Enclose a patch for new WAL records for relcache invalidation. > > I don't think this works. RelationCacheInitFileInvalidate is executed > post-commit, which means that there's a window between commit and where > you propose to write the WAL entry. A crash and restart in that > interval would leave the catalog changes committed, but not reflected > into pg_internal.init. Surely you are pointing out a bug, no? If a backend did crash, the init file would be wrong and we'd get exactly the same wrong relfilenode errors we got after that PITR. The issue must surely be that the patch isn't wrong per se, just that RelationCacheInitFileInvalidate is called too late and that requires an additional fix. Are we certain that a crash between commit and invalidation will cause a PANIC that takes down the server? Doesn't look like its in a critical section to me. > I think we're probably better off to just forcibly remove the init file > during post-recovery cleanup. The easiest place to do this might be > BuildFlatFiles, which has to scan pg_database anyway ... I can do this - I don't have a problem there, but the above issue just occurred to me so I wonder now if its the right thing to do. PITR will be always-safe but normal operation might not be. -- Simon Riggs EnterpriseDB http://www.enterprisedb.com
"Simon Riggs" <simon@2ndquadrant.com> writes: > On Wed, 2006-11-01 at 12:05 -0500, Tom Lane wrote: >> I don't think this works. > Surely you are pointing out a bug, no? > If a backend did crash, the init file would be wrong and we'd get > exactly the same wrong relfilenode errors we got after that PITR. Yeah, which is the same bug we've got now. They're both WAL-replay- doesn't-fix-the-init-file cases. > The issue must surely be that the patch isn't wrong per se, just that > RelationCacheInitFileInvalidate is called too late and that requires an > additional fix. No. In the non-crash situation there is sufficient interlocking to avoid a problem, and I feel no desire to redesign that mechanism. Trying to do it before commit would create its own issues, anyway: someone could install a new init file before you finish committing. regards, tom lane
On Wed, 2006-11-01 at 12:05 -0500, Tom Lane wrote: > I think we're probably better off to just forcibly remove the init file > during post-recovery cleanup. The easiest place to do this might be > BuildFlatFiles, which has to scan pg_database anyway ... Patch enclosed. Clean apply to HEAD, make check OK, plus restart check. No specific PITR test, since same code path. -- Simon Riggs EnterpriseDB http://www.enterprisedb.com
Вложения
"Simon Riggs" <simon@2ndquadrant.com> writes: > On Wed, 2006-11-01 at 12:05 -0500, Tom Lane wrote: >> I think we're probably better off to just forcibly remove the init file >> during post-recovery cleanup. The easiest place to do this might be >> BuildFlatFiles, which has to scan pg_database anyway ... > Patch enclosed. Applied to HEAD and 8.1. It doesn't work in 8.0 though, because flatfiles.c doesn't exist. AFAIR there isn't any startup-time scan of pg_database at all before 8.1. Not sure if it's worth coming up with a whole new patch for that branch. regards, tom lane