Re: [GSoC2014] Patch ALTER TABLE ... SET LOGGED - Mailing list pgsql-hackers

<div dir="ltr"><div class="gmail_extra"><br />On Wed, Jul 23, 2014 at 5:48 PM, Fabrízio de Royes Mello <<a
href="mailto:fabriziomello@gmail.com">fabriziomello@gmail.com</a>>wrote:<br />><br />><br />> On Tue, Jul
22,2014 at 3:29 PM, Fabrízio de Royes Mello <<a
href="mailto:fabriziomello@gmail.com">fabriziomello@gmail.com</a>>wrote:<br /> > ><br />> > On Tue, Jul
22,2014 at 12:01 PM, Fabrízio de Royes Mello <<a
href="mailto:fabriziomello@gmail.com">fabriziomello@gmail.com</a>>wrote:<br />> > ><br />> > ><br
/>>> ><br /> > > > On Thu, Jul 17, 2014 at 8:02 PM, Andres Freund <<a
href="mailto:andres@2ndquadrant.com">andres@2ndquadrant.com</a>>wrote:<br />> > > ><br />> > >
>> That means should I "FlushRelationBuffers(rel)" before change the<br /> > > > > >
relpersistence?<br/>> > > ><br />> > > > I don't think that'd help. I think what this means
thatyou simply<br />> > > > cannot change the relpersistence of the old relation before the heap<br /> >
>> > swap is successful. So I guess it has to be something like (pseudocode):<br />> > > ><br
/>>> > > OIDNewHeap = make_new_heap(..);<br />> > > > newrel = heap_open(OIDNewHeap, AEL);<br
/>> > > ><br />> > > > /*<br />> > > >  * Change the temporary relation to be
unlogged/logged.We have to do<br />> > > >  * that here so buffers for the new relfilenode will have the
right<br/> > > > >  * persistency set while the original filenode's buffers won't get read<br />> >
>>  * in with the wrong (i.e. new) persistency setting. Otherwise a<br />> > > >  * rollback after
therewrite would possibly result with buffers for the<br /> > > > >  * original filenode having the wrong
persistencysetting.<br />> > > >  *<br />> > > >  * NB: This relies on swap_relation_files()
alsoswapping the<br />> > > >  * persistency. That wouldn't work for pg_class, but that can't be<br /> >
>> >  * unlogged anyway.<br />> > > >  */<br />> > > >
AlterTableChangeCatalogToLoggedOrUnlogged(newrel);<br/>> > > > FlushRelationBuffers(newrel);<br />> >
>> /* copy heap data into newrel */<br /> > > > > finish_heap_swap();<br />> > > ><br
/>>> > > And then in swap_relation_files() also copy the persistency.<br />> > > ><br />>
>> ><br />> > > > That's the best I can come up right now at least.<br /> > > > ><br
/>>> ><br />> > > Isn't better if we can set the relpersistence as an argument to "make_new_heap"
?<br/>> > ><br />> > > I'm thinking to change the make_new_heap:<br /> > > ><br />> >
>From:<br />> > ><br />> > >  make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, bool forcetemp,<br
/>>> >                LOCKMODE lockmode)<br />> > ><br />> > > To:<br /> > > ><br
/>>> >  make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, char relpersistence,<br />> > >            
  LOCKMODE lockmode)<br />> > ><br />> > > That way we can create the new heap with the appropriate
relpersistence,so in the swap_relation_files also copy the persistency, of course.<br /> > > ><br />> >
>And after copy the heap data to the new table (ATRewriteTable) change relpersistence of the OldHeap's indexes,
becausein the "finish_heap_swap" they'll be rebuild.<br />> > ><br /> > > > Thoughts?<br />> >
><br/>> ><br />> > The attached patch implement my previous idea based on Andres thoughts.<br />>
><br/>><br />> I don't liked the last version of the patch, especially this part:<br /> ><br />> +      
    /* check if SetUnlogged or SetLogged exists in subcmds */<br />> +            for(pass = 0; pass <
AT_NUM_PASSES;pass++)<br />> +            {<br />> +                List         *subcmds =
tab->subcmds[pass];<br/> > +                ListCell    *lcmd;<br />> +<br />> +                if (subcmds
==NIL)<br />> +                    continue;<br />> +<br />> +                foreach(lcmd, subcmds)<br />>
+               {<br /> > +                    AlterTableCmd *cmd = (AlterTableCmd *) lfirst(lcmd);<br />> +<br
/>>+                    if (cmd->subtype == AT_SetUnLogged || cmd->subtype == AT_SetLogged)<br />> +      
            {<br /> > +                        /*<br />> +                         * Change the temporary
relationto be unlogged/logged. We have to do<br />> +                         * that here so buffers for the new
relfilenodewill have the right<br /> > +                         * persistency set while the original filenode's
bufferswon't get read<br />> +                         * in with the wrong (i.e. new) persistency setting. Otherwise
a<br/>> +                         * rollback after the rewrite would possibly result with buffers for the<br /> >
+                        * original filenode having the wrong persistency setting.<br />> +                        
*<br/>> +                         * NB: This relies on swap_relation_files() also swapping the<br /> > +        
               * persistency. That wouldn't work for pg_class, but that can't be<br />> +                         *
unloggedanyway.<br />> +                         */<br />> +                        if (cmd->subtype ==
AT_SetUnLogged)<br/> > +                            newrelpersistence = RELPERSISTENCE_UNLOGGED;<br />> +<br
/>>+                        isSetLoggedUnlogged = true;<br />> +                    }<br />> +              
 }<br/>> +            }<br /> ><br />><br />> So I did a refactoring adding new items to AlteredTableInfo
topass the information through the phases.<br />><br /><br />Hi all,<br /><br />There are something that should I do
onthis patch yet?<br /><br />Regards<br /><br />--<br />Fabrízio de Royes Mello<br />Consultoria/Coaching PostgreSQL<br
/>>>Timbira: <a href="http://www.timbira.com.br">http://www.timbira.com.br</a><br />>> Blog sobre TI: <a
href="http://fabriziomello.blogspot.com">http://fabriziomello.blogspot.com</a><br/> >> Perfil Linkedin: <a
href="http://br.linkedin.com/in/fabriziomello">http://br.linkedin.com/in/fabriziomello</a><br/>>> Twitter: <a
href="http://twitter.com/fabriziomello">http://twitter.com/fabriziomello</a></div></div>