On Fri, Dec 11, 2015 at 4:54 PM, Michael Paquier <michael.paqu...@gmail.com> wrote: > On Fri, Dec 11, 2015 at 4:27 AM, Andres Freund <and...@anarazel.de> wrote: >> On 2015-12-10 18:36:32 +0100, Andres Freund wrote: >>> On 2015-12-10 12:19:12 -0500, Robert Haas wrote: >>> > > The real problem there imo isn't that the copy_relation_data() doesn't >>> > > deal with 0 block tables, but that ATExecSetTableSpace() doesn't have a >>> > > unlogged table specific codepath like heap_create_with_catalog() has. >>> > >>> > It looks to me like somewhere we need to do log_smgrcreate(..., >>> > INIT_FORKNUM) in the unlogged table case. >>> >>> Yes. >>> >>> > RelationCreateStorage() >>> > skips this for the main forknum of an unlogged table, which seems OK, >>> > but there's nothing that even attempts it for the init fork, which >>> > does not seem OK. >>> >>> We unfortunately can't trivially delegate that work to >>> RelationCreateStorage(). E.g. heap_create() documents that only the main >>> fork is created :( >>> >>> > I guess that logic should happen in >>> > ATExecSetTableSpace just after smgrcreate(dstrel, forkNum, false). >>> >>> Looks like it's the easiest place. >> >>> > > A second problem is that the smgrimmedsync() in copy_relation_data() >>> > > isn't called for the init fork of unlogged relations, even if it needs >>> > > to. >> >> Here's a patch doing that. It's not yet fully polished, but I wanted to >> get it out, because I noticed one thing: >> >> In ATExecSetTableSpace(), for !main forks, we currently call >> smgrcreate(), but not log_smgrcreate(). Even for PERSISTENT >> relations. That seems a bit odd to me. It currently seems to be without >> further consequence because, if there's actual data in the fork, we'll >> just create the relation in _mdfd_getseg(); or we can cope with the >> relation not being there. But to me that feels wrong. >> >> It seems better to do the log_smgrcreate() for RELPERSISTENCE_PERMANENT, >> not just INIT_FORKNUM. What do you guys think? > > This fixes the problem in my environment. > > + if (rel->rd_rel->relpersistence == > RELPERSISTENCE_PERMANENT || > + (rel->rd_rel->relpersistence == > RELPERSISTENCE_UNLOGGED && > + forkNum == INIT_FORKNUM)) > + log_smgrcreate(&newrnode, forkNum); > There should be a XLogIsNeeded() check as well. Removing the check on > RELPERSISTENCE_UNLOGGED is fine as well... Not mandatory though :) > > + * The init fork for an unlogged relation in many respects has to be > + * treated the same as normal relation, changes need to be WAL > logged and > + * it needs to be synced to disk. > + */ > + copying_initfork = relpersistence == RELPERSISTENCE_UNLOGGED && > + forkNum == INIT_FORKNUM; > Here as well just a check on INIT_FORKNUM would be fine.
Should we consider this bug a 9.5 blocker? I feel uneasy with the fact of releasing a new major version knowing that we know some bugs on it, and this one is not cool so I have added it in the list of open items. We know the problem and there is a patch, so this is definitely solvable. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers