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

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.

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