Thread
-
crash-recovery replay of CREATE TABLESPACE is broken in HEAD
Tom Lane <tgl@sss.pgh.pa.us> — 2010-07-11T23:37:18Z
I managed to crash the executor in the tablespace.sql test while working on a 9.1 patch, and discovered that the postmaster fails to recover from that. The end of postmaster.log looks like LOG: all server processes terminated; reinitializing LOG: database system was interrupted; last known up at 2010-07-11 19:30:07 EDT LOG: database system was not properly shut down; automatic recovery in progress LOG: consistent recovery state reached at 0/EE26F30 LOG: redo starts at 0/EE26F30 FATAL: directory "/home/postgres/pgsql/src/test/regress/testtablespace/PG_9.1_201004261" already in use as a tablespace CONTEXT: xlog redo create ts: 127158 "/home/postgres/pgsql/src/test/regress/testtablespace" LOG: startup process (PID 13914) exited with exit code 1 LOG: aborting startup due to startup process failure It looks to me like those well-intentioned recent changes in this area broke the crash-recovery case. Not good. regards, tom lane
-
Re: crash-recovery replay of CREATE TABLESPACE is broken in HEAD
Bruce Momjian <bruce@momjian.us> — 2010-07-18T05:22:25Z
Tom Lane wrote: > I managed to crash the executor in the tablespace.sql test while working > on a 9.1 patch, and discovered that the postmaster fails to recover > from that. The end of postmaster.log looks like > > LOG: all server processes terminated; reinitializing > LOG: database system was interrupted; last known up at 2010-07-11 19:30:07 EDT > LOG: database system was not properly shut down; automatic recovery in progress > LOG: consistent recovery state reached at 0/EE26F30 > LOG: redo starts at 0/EE26F30 > FATAL: directory "/home/postgres/pgsql/src/test/regress/testtablespace/PG_9.1_201004261" already in use as a tablespace > CONTEXT: xlog redo create ts: 127158 "/home/postgres/pgsql/src/test/regress/testtablespace" > LOG: startup process (PID 13914) exited with exit code 1 > LOG: aborting startup due to startup process failure > > It looks to me like those well-intentioned recent changes in this area > broke the crash-recovery case. Not good. Sorry for the delay. I didn't realize this was my code that was broken until Heikki told me via IM. The bug is that we can't replay mkdir()/symlink() and assume those will always succeed. I looked at the createdb redo code and it basically drops the directory before creating it. The tablespace directory/symlink setup is more complex, so I just wrote the attached patch to trigger a redo-'delete' tablespace operation before the create tablespace redo operation. Ignoring mkdir/symlink creation failure is not an option because the symlink might point to some wrong location or something. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + None of us is going to be here forever. +
-
Re: crash-recovery replay of CREATE TABLESPACE is broken in HEAD
Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2010-07-18T05:56:02Z
On 18/07/10 08:22, Bruce Momjian wrote: > The bug is that we can't replay mkdir()/symlink() and assume those will > always succeed. I looked at the createdb redo code and it basically > drops the directory before creating it. > > The tablespace directory/symlink setup is more complex, so I just wrote > the attached patch to trigger a redo-'delete' tablespace operation > before the create tablespace redo operation. Redoing a drop talespace assumes the tablespace directory is empty, which it necessarily isn't when redoing a create tablespace command: postgres=# CREATE TABLESPACE t LOCATION '/tmp/t'; CREATE TABLESPACE postgres=# CREATE TABLE tfoo (id int4) TABLESPACE t; CREATE TABLE postgres=# \q $ killall -9 postmaster $ bin/postmaster -D data LOG: database system was interrupted; last known up at 2010-07-18 08:48:32 EEST LOG: database system was not properly shut down; automatic recovery in progress LOG: consistent recovery state reached at 0/5E889C LOG: redo starts at 0/5E889C FATAL: tablespace 16402 is not empty CONTEXT: xlog redo create ts: 16402 "/tmp/t" LOG: startup process (PID 5987) exited with exit code 1 LOG: aborting startup due to startup process failure Also, casting the xl_tblspc_create_rec as xl_tblspc_drop_rec is a bit questionable. It works because both structs begin with the tablespace Oid, but it doesn't look right, and can break in hard-to-notice ways in the future if the structure of those structs change in the future. > Ignoring mkdir/symlink creation failure is not an option because the > symlink might point to some wrong location or something. Maybe you should check that it points to the right location? Or drop and recreate the symlink, and ignore failure at mkdir. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
-
Re: crash-recovery replay of CREATE TABLESPACE is broken in HEAD
Tom Lane <tgl@sss.pgh.pa.us> — 2010-07-18T14:15:55Z
Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes: > Maybe you should check that it points to the right location? Or drop and > recreate the symlink, and ignore failure at mkdir. More specifically, ignore EEXIST failure when replaying mkdir. Anything else is still a problem. regards, tom lane
-
Re: crash-recovery replay of CREATE TABLESPACE is broken in HEAD
Bruce Momjian <bruce@momjian.us> — 2010-07-18T15:54:51Z
Tom Lane wrote: > Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes: > > Maybe you should check that it points to the right location? Or drop and > > recreate the symlink, and ignore failure at mkdir. > > More specifically, ignore EEXIST failure when replaying mkdir. Anything > else is still a problem. Thanks for the help. I tried to find somewhere else in our recovery code that was similar but didn't find anything. The attached patch does as suggested. I added the recovery code to the create tablespace function so I didn't have to duplicate all the code that computes the path names. Attached. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + None of us is going to be here forever. +
-
Re: crash-recovery replay of CREATE TABLESPACE is broken in HEAD
Bruce Momjian <bruce@momjian.us> — 2010-07-18T16:05:44Z
Bruce Momjian wrote: > Tom Lane wrote: > > Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes: > > > Maybe you should check that it points to the right location? Or drop and > > > recreate the symlink, and ignore failure at mkdir. > > > > More specifically, ignore EEXIST failure when replaying mkdir. Anything > > else is still a problem. > > Thanks for the help. I tried to find somewhere else in our recovery > code that was similar but didn't find anything. > > The attached patch does as suggested. I added the recovery code to the > create tablespace function so I didn't have to duplicate all the code > that computes the path names. > > Attached. Uh, another question. Looking at the createdb recovery, I see: /* * Our theory for replaying a CREATE is to forcibly drop the target * subdirectory if present, then re-copy the source data. This may be * more work than needed, but it is simple to implement. */ if (stat(dst_path, &st) == 0 && S_ISDIR(st.st_mode)) { if (!rmtree(dst_path, true)) ereport(WARNING, (errmsg("some useless files may be left behind in old database directory \"%s\"", dst_path))); } Should I be using rmtree() on the mkdir target? Also, the original tablespace recovery code did not drop the symlink first. I assume that was not a bug only because we don't support moving tablespaces: - /* Create the symlink if not already present */ - linkloc = (char *) palloc(OIDCHARS + OIDCHARS + 1); - sprintf(linkloc, "pg_tblspc/%u", xlrec->ts_id); - - if (symlink(location, linkloc) < 0) - { - if (errno != EEXIST) - ereport(ERROR, - (errcode_for_file_access(), - errmsg("could not create symbolic link \"%s\": %m", - linkloc))); - } Still, it seems logical to unlink it before creating it. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + None of us is going to be here forever. + -
Re: crash-recovery replay of CREATE TABLESPACE is broken in HEAD
Bruce Momjian <bruce@momjian.us> — 2010-07-19T05:02:08Z
Bruce Momjian wrote: > > The attached patch does as suggested. I added the recovery code to the > > create tablespace function so I didn't have to duplicate all the code > > that computes the path names. > > > > Attached. > > Uh, another question. Looking at the createdb recovery, I see: > > /* > * Our theory for replaying a CREATE is to forcibly drop the target > * subdirectory if present, then re-copy the source data. This may be > * more work than needed, but it is simple to implement. > */ > if (stat(dst_path, &st) == 0 && S_ISDIR(st.st_mode)) > { > if (!rmtree(dst_path, true)) > ereport(WARNING, > (errmsg("some useless files may be left behind in old database directory \"%s\"", > dst_path))); > } > > Should I be using rmtree() on the mkdir target? > > Also, the original tablespace recovery code did not drop the symlink > first. I assume that was not a bug only because we don't support moving > tablespaces: For consistency with CREATE DATABASE recovery and for reliablity, I coded the rmtree() call instead. Patch attached. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + None of us is going to be here forever. + -
Re: crash-recovery replay of CREATE TABLESPACE is broken in HEAD
Bruce Momjian <bruce@momjian.us> — 2010-07-20T18:21:47Z
Bruce Momjian wrote: > Bruce Momjian wrote: > > > The attached patch does as suggested. I added the recovery code to the > > > create tablespace function so I didn't have to duplicate all the code > > > that computes the path names. > > > > > > Attached. > > > > Uh, another question. Looking at the createdb recovery, I see: > > > > /* > > * Our theory for replaying a CREATE is to forcibly drop the target > > * subdirectory if present, then re-copy the source data. This may be > > * more work than needed, but it is simple to implement. > > */ > > if (stat(dst_path, &st) == 0 && S_ISDIR(st.st_mode)) > > { > > if (!rmtree(dst_path, true)) > > ereport(WARNING, > > (errmsg("some useless files may be left behind in old database directory \"%s\"", > > dst_path))); > > } > > > > Should I be using rmtree() on the mkdir target? > > > > Also, the original tablespace recovery code did not drop the symlink > > first. I assume that was not a bug only because we don't support moving > > tablespaces: > > For consistency with CREATE DATABASE recovery and for reliablity, I > coded the rmtree() call instead. Patch attached. Attached patch applied to HEAD and 9.0. 9.0 open item moved to completed. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + None of us is going to be here forever. +