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

Hi Fabrízio,

thanks for the speedy new version.

Re: Fabrízio de Royes Mello 2014-07-15 <CAFcNs+opBuHjUo1sOATHv8zqcOwqp-yeRjFoo15v1xPXSpCdDw(at)mail(dot)gmail(dot)com>
> > > > > +
> > > > > + 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

Oh there was one "not" too much for me, sorry. Anyway, this isn't
relevant anymore. :)

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

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

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

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.

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

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

Yes. It was just a minor nitpick anyway.

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

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

Christoph
--
cb(at)df7cb(dot)de | http://www.df7cb.de/