Re: Error with index on unlogged table

On Thu, Dec 10, 2015 at 11:32 AM, Andres Freund <andres(at)anarazel(dot)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