On Thu, Dec 10, 2015 at 11:32 AM, Andres Freund <and...@anarazel.de> wrote: > I've, pushed the fix for the promotion related issue. I'm afraid that > the ALTER TABLE <unlogged-table> SET TABLESPACE issue is a bit bigger > than it though.
I think that I would have preferred to fix this by flushing unlogged buffers in bulk at the end of recovery, rather than by flushing them as they are generated. This approach seems needlessly inefficient. Maybe it doesn't matter, but you made the same sort of decision with the find_multixact_start() fix: flushing sooner instead of working harder to make the delayed flush safe. I think that's a bad direction in general, and that it will bite us. Nonetheless, I've been unresponsive on this thread, and it's certainly better to have fixed the bug in a way that isn't what I would prefer than to have not fixed it. > Robert, to catch you up: The isssue, discovered by Michael somewhere in > this thread, is that copy_relation_data(), used by SET TABLESPACE, does > not deal with unlogged tables. Hmm. [ ... ] > But that's not sufficient for a bunch of reasons: > > First, and that's the big one, that approach doesn't guarantee that the > file will be created in the new tablespace if the file does not have any > blocks. Like e.g. the heap relation itself. This will lead to errors > like: > ERROR: 58P01: could not open file > "pg_tblspc/16384/PG_9.6_201511071/12384/16438": No such file or > LOCATION: mdopen, md.c:602 > > 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. 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. I guess that logic should happen in ATExecSetTableSpace just after smgrcreate(dstrel, forkNum, false). > 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. That seems easy enough to fix. Can't we just sync all forks at that location for permanent relations, and additionally sync the init fork only for unlogged relations? -- 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
