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

On Tue, Jul 22, 2014 at 12:01 PM, Fabrízio de Royes Mello <
fabriziomello(at)gmail(dot)com> wrote:
>
>
>
> On Thu, Jul 17, 2014 at 8:02 PM, Andres Freund <andres(at)2ndquadrant(dot)com>
wrote:
> >
> > > 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.
> >
>
> Isn't better if we can set the relpersistence as an argument to
"make_new_heap" ?
>
> I'm thinking to change the make_new_heap:
>
> From:
>
> make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, bool forcetemp,
> LOCKMODE lockmode)
>
> To:
>
> make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, char relpersistence,
> LOCKMODE lockmode)
>
> That way we can create the new heap with the appropriate relpersistence,
so in the swap_relation_files also copy the persistency, of course.
>
> And after copy the heap data to the new table (ATRewriteTable) change
relpersistence of the OldHeap's indexes, because in the "finish_heap_swap"
they'll be rebuild.
>
> Thoughts?
>

The attached patch implement my previous idea based on Andres thoughts.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog sobre TI: http://fabriziomello.blogspot.com
>> Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello