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

On Wed, Jul 23, 2014 at 5:48 PM, Fabrízio de Royes Mello <
fabriziomello(at)gmail(dot)com> wrote:
>
>
> On Tue, Jul 22, 2014 at 3:29 PM, Fabrízio de Royes Mello <
fabriziomello(at)gmail(dot)com> wrote:
> >
> > 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.
> >
>
> 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