On 2014-07-16 20:45:15 -0300, Fabrízio de Royes Mello wrote: > On Wed, Jul 16, 2014 at 7:26 PM, Andres Freund <and...@2ndquadrant.com> > wrote: > > > > On 2014-07-16 20:53:06 +0200, Andres Freund wrote: > > > On 2014-07-16 20:25:42 +0200, Andres Freund wrote: > > > > Hi, > > > > > > > > I quickly looked at this patch and I think there's major missing > pieces > > > > around buffer management and wal logging. > > > > > > > > a) Currently buffers that are in memory marked as > > > > permanent/non-permanent aren't forced out to disk/pruned from > cache, > > > > not even when they're dirty. > > > > b) When converting from a unlogged to a logged table the relation > needs > > > > to be fsynced. > > > > c) Currently a unlogged table changed into a logged one will be > > > > corrupted on a standby because its contents won't ever be WAL > logged. > > > > > > Forget that, didn't notice that you're setting tab->rewrite = true. > > > > So, while that danger luckily isn't there I think there's something > > similar. Consider: > > > > CREATE TABLE blub(...); > > INSERT INTO blub ...; > > > > BEGIN; > > ALTER TABLE blub SET UNLOGGED; > > ROLLBACK; > > > > The rewrite will read in the 'old' contents - but because it's done > > after the pg_class.relpersistence is changed they'll all not be marked > > as BM_PERMANENT in memory. Then the ALTER TABLE is rolled back, > > including the relpersistence setting. Which will unfortunately leave > > pages with the wrong persistency setting in memory, right? > > > > That means should I "FlushRelationBuffers(rel)" before change the > relpersistence?
I don't think that'd help. I think what this means that you simply cannot change the relpersistence of the old relation before the heap swap is successful. So I guess it has to be something like (pseudocode): OIDNewHeap = make_new_heap(..); newrel = heap_open(OIDNewHeap, AEL); /* * Change the temporary relation to be unlogged/logged. We have to do * that here so buffers for the new relfilenode will have the right * persistency set while the original filenode's buffers won't get read * in with the wrong (i.e. new) persistency setting. Otherwise a * rollback after the rewrite would possibly result with buffers for the * original filenode having the wrong persistency setting. * * NB: This relies on swap_relation_files() also swapping the * persistency. That wouldn't work for pg_class, but that can't be * unlogged anyway. */ AlterTableChangeCatalogToLoggedOrUnlogged(newrel); FlushRelationBuffers(newrel); /* copy heap data into newrel */ finish_heap_swap(); And then in swap_relation_files() also copy the persistency. That's the best I can come up right now at least. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
