Re: [GSoC2014] Patch ALTER TABLE ... SET LOGGED

From: Andres Freund <andres(at)2ndquadrant(dot)com>
To: Fabrízio de Royes Mello <fabriziomello(at)gmail(dot)com>
Cc: Christoph Berg <cb(at)df7cb(dot)de>, Pgsql Hackers <pgsql-hackers(at)postgresql(dot)org>, Stephen Frost <sfrost(at)snowman(dot)net>
Subject: Re: [GSoC2014] Patch ALTER TABLE ... SET LOGGED
Date: 2014-07-17 23:02:20
Message-ID: 20140717230220.GK21370@awork2.anarazel.de
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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 <andres(at)2ndquadrant(dot)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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Peter Geoghegan 2014-07-18 01:18:41 Making joins involving ctid work for the benefit of UPSERT
Previous Message Josh Berkus 2014-07-17 22:56:20 Re: Set new system identifier using pg_resetxlog