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

On Tue, Jul 15, 2014 at 3:04 PM, Christoph Berg <cb(at)df7cb(dot)de> wrote:
>
> Hi Fabrízio,
>
> thanks for the speedy new version.
>

You're welcome... If all happen ok I would like to have this patch commited
before the GSoC2014 ends.

> > > I've just tried some SET (UN)LOGGED operations with altering column
> > > types in the same operation, that works. But:
> > >
> > > Yes, you should use the existing table rewriting machinery, or at
> > > least clearly document (in comments) why it doesn't work for you.
> > >
> > > Also looking at ATController, there's a wqueue mechanism to queue
> > > catalog updates. You should probably use this, too, or again document
> > > why it doesn't work for you.
> > >
> >
> > This works... fixed!
>
> Thanks.
>
> What about the wqueue mechanism, though? Isn't that made exactly for
> the kind of catalog updates you are doing?
>

Works, but this mechanism create a new entry in pg_class for toast, so it's
a little different than use the cluster_rel that generate a new
relfilenode. The important is both mechanisms create new datafiles.

> > > You miss the symmetric case the other way round. When changing a table
> > > to unlogged, you need to make sure no other permanent table is
> > > referencing our table.
> > >
> >
> > Ohh yeas... sorry... you're completely correct... fixed!
>
> Can you move ATPrepSetUnLogged next to ATPrepSetLogged as both
> reference AlterTableSetLoggedCheckForeignConstraints now, and fix the
> comment on ATPrepSetUnLogged to also mention FKs? I'd also reiterate
> my proposal to merge these into one function, given they are now doing
> the same checks.
>

Merged both to a single ATPrepSetLoggedOrUnlogged(Relation rel, bool
toLogged);

> In AlterTableSetLoggedCheckForeignConstraints, move "relfk =
> relation_open..." out of the "if" because it's duplicated, and also for
> symmetry with relation_close().
>

But they aren't duplicated... the first opens "con->confrelid" and the
other opens "con->conrelid" according "toLogged" branch.

> The function needs comments. It is somewhat clear that
> self-referencing FKs will be skipped, but the two "if (toLogged)"
> branches should be annotated to say which case they are really about.
>

Fixed.

> Instead of just switching the argument order in the errmsg arguments,
> the error text should be updated to read "table %s is referenced
> by permanent table %s". At the moment the error text is wrong because
> the table logged1 is not yet unlogged:
>
> +ALTER TABLE logged1 SET UNLOGGED;
> +ERROR: table logged2 references unlogged table logged1
>
> -> table logged1 is referenced by permanent table logged2
>

Sorry... my mistake... fixed

> Compared to v3, you've removed a lot of "SELECT t.relpersistence..."
> from the regression tests, was that intended?
>

I removed because they are not so useful than I was thinking before.
Actually they just bloated our test cases.

> I think the tests could also use a bit more comments, notably the
> commands that are expected to fail. So far I haven't tried to read
> them but trusted that they did the right thing. (Though with the
> SELECTs removed, it's pretty readable now.)
>

Added some comments.

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