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

Robert Haas wrote:

> Hmm. I confess to not having paid enough attention to this,

Sorry about that. I guess I should somehow flag threads "I'm planning
to commit this" so that other people can review stuff carefully.

> but:
>
> 1. Loggedness is not a word. I think that "persistence" or
> "relpersistence" would be better.

Yeah, AFAICS this is only used in the variable chgLoggedness and a
couple of functions. I don't think we're tense about unwordness of
words we use in source code (as opposed to error messages and docs), and
we have lots of russianisms left by Vadim and others, so I didn't see it
as a serious issue. But then I'm not a native english speaker and I'm
not bothered by it. OTOH I came up with "loggedness" on my own -- you
wouldn't have seen it even in Fabrizio's latest version of the patch.

Who knows, it might become a real english word one day.

You want me to change that to chgPersistence and so on? No prob, just
LMK.

> 2. The patch seems to think that it can sometimes be safe to change
> the relpersistence of an existing relation. Unless you can be sure
> that no buffers can possibly be present in shared_buffers and nobody
> will use an existing relcache entry to read a new one in, it's not,
> because the buffers won't have the right BM_PERSISTENT marking. I'm
> very nervous about the fact that this patch seems not to have touched
> bufmgr.c, but maybe I'm missing something.

Right. Andres pointed this out previously, and the patch was updated
consequently. The only remaining case in which we do that, again AFAIR,
is for indexes, just after the table has been rewritten and just before
the indexes are reindexed. There should be no buffer access of the old
indexes at that point, so there would be no buffer marked with the wrong
flag. Also, the table is locked access-exclusively (is that a word?),
so no other transaction could possibly be reading buffers for its
indexes.

I pointed out, in the email just before pushing the patch, that perhaps
we should pass down the new relpersistence flag into finish_heap_swap,
and from there we could pass it down to reindex_index() which is where
it would be needed. I'm not sure it's worth the trouble, but I think we
can still ask Fabrizio to rework that part.

Maybe it is me missing something.

BTW why is it that index_build() checks the heap's relpersistence flag
rather than the index'?

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