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

On Mon, Jul 14, 2014 at 3:31 PM, Christoph Berg <cb(at)df7cb(dot)de> wrote:
>
> Oh I wasn't aware of the wiki page, I had just read the old thread.
> Thanks for the pointer.
>

:-)

Thanks again for your review!

> > > > diff --git a/doc/src/sgml/ref/alter_table.sgml
> > b/doc/src/sgml/ref/alter_table.sgml
> > > > index 69a1e14..424f2e9 100644
> > > > --- a/doc/src/sgml/ref/alter_table.sgml
> > > > +++ b/doc/src/sgml/ref/alter_table.sgml
> > > > @@ -58,6 +58,7 @@ ALTER TABLE [ IF EXISTS ] <replaceable
> > class="PARAMETER">name</replaceable>
> > > > ENABLE REPLICA RULE <replaceable
> > class="PARAMETER">rewrite_rule_name</replaceable>
> > > > ENABLE ALWAYS RULE <replaceable
> > class="PARAMETER">rewrite_rule_name</replaceable>
> > > > CLUSTER ON <replaceable
class="PARAMETER">index_name</replaceable>
> > > > + SET {LOGGED | UNLOGGED}
> > > > SET WITHOUT CLUSTER
> > > > SET WITH OIDS
> > > > SET WITHOUT OIDS
> > >
> > > This must not be between the two CLUSTER lines. I think the best spot
> > > would just be one line down, before SET WITH OIDS.
> >
> > Fixed.
>
> The (long) SET LOGGED paragraph is still between CLUSTER and SET
> WITHOUT CLUSTER.
>

Fixed.

> > > This grammar bug pops up consistently: This form *changes*...
> > >
> >
> > Fixed.
>
> Two more:
>
> + * The AlterTableChangeCatalogToLoggedOrUnlogged function perform the
> + * The AlterTableChangeIndexesToLoggedOrUnlogged function scan all
indexes
>

Fixed.

> > > > relation_close(rel, NoLock);
> > > > +
> > > > + if (pass == AT_PASS_SET_LOGGED_UNLOGGED)
> > > > +
> > ATPostAlterSetLoggedUnlogged(RelationGetRelid(rel));
> > >
> > > This must be done before relation_close() which releases all locks.
>
> You didn't address that. I'm not sure about it, but either way, this
> deserves a comment on the lock level necessary.
>

Actually relation_close(rel, NoLock) don't release the locks.

See src/backend/access/heap/heapam.c:1167

> > > Moreover, I think you can get rid of that extra PASS here.
> > > AT_PASS_ALTER_TYPE has its own pass because you can alter several
> > > columns in a single ALTER TABLE statement, but you can have only one
> > > SET (UN)LOGGED, so you can to the cluster_rel() directly in
> > > AlterTableSetLoggedOrUnlogged() (unless cluster_rel() is too intrusive
> > > and would interfere with other ALTER TABLE operations in this command,
> > > no idea).
> > >
> >
> > I had some troubles here so I decided to do in that way, but I confess
I'm
> > not comfortable with this implementation. Looking more carefully on
> > tablecmds.c code, at the ATController we have three phases and the
third is
> > 'scan/rewrite tables as needed' so my doubt is if can I use it instead
of
> > call 'cluster_rel'?
>
> 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!

> > > Here's the big gotcha: Just like SET LOGGED must check for outgoing
> > > FKs to unlogged tables, SET UNLOGGED must check for incoming FKs from
> > > permanent tables. This is missing.
> > >
> >
> > I don't think so... we can create an unlogged table with a FK referring
to
> > a regular table...
> > ... but is not possible create a FK from a regular table referring to an
> > unlogged table:
> > ... and a FK from an unlogged table referring other unlogged table
works:
> > So we must take carefull just when changing an unlogged table to a
regular
> > table.
> >
> > Am I correct or I miss something?
>
> 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!

> > > > +AlterTableChangeCatalogToLoggedOrUnlogged(Relation rel, Relation
> > relrelation, bool toLogged)
>
> You are using "relrelation" and "relrel". I'd change all occurrences
> to "relrelation" because that's also used elsewhere.
>

Fixed.

> > > The comment on heap_open() suggests that you could directly invoke
> > > relation_open() because you know this is a toast table, similarly for
> > > index_open(). (I can't say which is better style.)
> > >
> >
> > I don't know which is better style too... other opinions??
>
> Both are used several times in tablecmds.c, so both are probably fine.
> (Didn't check the contexts, though.)
>

Then we can leave that way. Is ok for you?

Greetings,

--
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