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

On Tue, Jul 22, 2014 at 3:29 PM, Fabrízio de Royes Mello <fabriziomello@gmail.com> wrote:
>
> On Tue, Jul 22, 2014 at 12:01 PM, Fabrízio de Royes Mello <fabriziomello@gmail.com> wrote:
> >
> >
> >
> > On Thu, Jul 17, 2014 at 8:02 PM, Andres Freund <andres@2ndquadrant.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.
>

I don't liked the last version of the patch, especially this part:

+            /* check if SetUnlogged or SetLogged exists in subcmds */
+            for(pass = 0; pass < AT_NUM_PASSES; pass++)
+            {
+                List         *subcmds = tab->subcmds[pass];
+                ListCell    *lcmd;
+
+                if (subcmds == NIL)
+                    continue;
+
+                foreach(lcmd, subcmds)
+                {
+                    AlterTableCmd *cmd = (AlterTableCmd *) lfirst(lcmd);
+
+                    if (cmd->subtype == AT_SetUnLogged || cmd->subtype == AT_SetLogged)
+                    {
+                        /*
+                         * 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.
+                         */
+                        if (cmd->subtype == AT_SetUnLogged)
+                            newrelpersistence = RELPERSISTENCE_UNLOGGED;
+
+                        isSetLoggedUnlogged = true;
+                    }
+                }
+            }

So I did a refactoring adding new items to AlteredTableInfo to pass the information through the phases.

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