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

On Wed, Jul 23, 2014 at 5:48 PM, Fabrízio de Royes Mello <fabriziomello@gmail.com> wrote:
>
>
> 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.
>

Hi all,

There are something that should I do on this patch yet?

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