On Thu, Nov 10, 2016 at 9:25 PM, Michael Paquier <michael.paqu...@gmail.com> wrote: > On Thu, Nov 10, 2016 at 10:52 PM, Kuntal Ghosh > <kuntalghosh.2...@gmail.com> wrote: >> On Thu, Nov 10, 2016 at 3:42 PM, Michael Paquier >> <michael.paqu...@gmail.com> wrote: >>> Nah. Looking at the code the fix is quite obvious. >>> heap_create_init_fork() is checking for XLogIsNeeded() to decide if >>> the INIT forknum should be logged or not. But this is wrong, it needs >>> to be done unconditionally to ensure that the relation gets created at >>> replay. >> >> I think that we should also update other *buildempty() functions as well. >> For example, if the table has a primary key, we'll encounter the error again >> for btree index. > > Good point. I just had a look at all the AMs: bloom, btree and SP-gist > are plainly broken. The others are fine. Attached is an updated patch. > > Here are the tests I have done with wal_level = minimal to test all the AMs: > \! rm -rf /Users/mpaquier/Desktop/tbsp/PG* > create extension bloom; > create extension btree_gist; > create extension btree_gin; > create tablespace popo location '/Users/mpaquier/Desktop/tbsp'; > set default_tablespace = popo; > create unlogged table foo (a int); > insert into foo values (generate_series(1,10000)); > create index foo_bt on foo(a); > create index foo_bloom on foo using bloom(a); > create index foo_gin on foo using gin (a); > create index foo_gist on foo using gist (a); > create index foo_brin on foo using brin (a); > create unlogged table foo_geo (a box); > insert into foo_geo values ('(1,2),(2,3)'); > create index foo_spgist ON foo_geo using spgist(a); > > -- crash PG > -- Insert some data > insert into foo values (1000000); > insert into foo_geo values ('(50,12),(312,3)'); > > This should create 8 INIT forks, 6 for the indexes and 2 for the > tables. On HEAD, only 3 are getting created and with the patch all of > them are.
The header comment for heap_create_init_fork() says this: /* * Set up an init fork for an unlogged table so that it can be correctly * reinitialized on restart. Since we're going to do an immediate sync, we * only need to xlog this if archiving or streaming is enabled. And the * immediate sync is required, because otherwise there's no guarantee that * this will hit the disk before the next checkpoint moves the redo pointer. */ Your patch causes the code not to match the comment any more. And the comment explains why at the time I wrote this code I thought it was OK to have the XLogIsNeeded() test in there, so it clearly needs updating. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers