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

Tom Lane wrote:
> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> > Agreed.  I am going over this patch, and the last bit I need to sort out
> > is the wording of these messages.  I have temporarily settled on this:
>
> >     ereport(ERROR,
> >             (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
> >              errmsg("cannot change logged status of table %s to logged",
> >                     RelationGetRelationName(rel)),
> >              errdetail("Table %s references unlogged table %s.",
> >                        RelationGetRelationName(rel),
> >                        RelationGetRelationName(relfk))));
>
> > Note the term "logged status" to talk about whether a table is logged or
> > not.  I thought about "loggedness" but I'm not sure english speakers are
> > going to love me for that.  Any other ideas there?
>
> Just say "cannot change status of table %s to logged".

I like this one, thanks.

> > Yeah, there is precedent for silently doing nothing.  We previously
> > threw warnings or notices, but nowadays even that is gone.  Throwing an
> > error definitely seems the wrong thing.
>
> Agreed, just do nothing if it's already the right setting.

Done that way.

Andres Freund wrote:

> Have you looked at the correctness of the patch itself? Last time I'd
> looked it has fundamental correctness issues. I'd outlined a possible
> solution, but I haven't looked since.

Yeah, Fabrizio had it passing the relpersistence down to make_new_heap,
so the transient table is created with the right setting.  AFAICS it's
good now.  I'm a bit uneasy about the way it changes indexes: it just
updates pg_class for them just before invoking the reindex in
finish_heap_swap.  I think it's correct as it stands though; at least,
the rewrite phase happens with the right setting, so that if there are
constraints being checked and these invoke index scans, such accesses
would not leave buffers with the wrong setting in shared_buffers.

Another option would be to pass the new relpersistence down to
finish_heap_swap, but that would be hugely complicated AFAICS.

Here's the updated patch.

--
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services