Re: [GSoC2014] Patch ALTER TABLE ... SET LOGGED - Mailing list pgsql-hackers
From | Alvaro Herrera |
---|---|
Subject | Re: [GSoC2014] Patch ALTER TABLE ... SET LOGGED |
Date | |
Msg-id | 20140822194547.GQ6343@eldon.alvh.no-ip.org Whole thread Raw |
In response to | Re: [GSoC2014] Patch ALTER TABLE ... SET LOGGED (Robert Haas <robertmhaas@gmail.com>) |
Responses |
Re: [GSoC2014] Patch ALTER TABLE ... SET LOGGED
Re: [GSoC2014] Patch ALTER TABLE ... SET LOGGED Re: [GSoC2014] Patch ALTER TABLE ... SET LOGGED |
List | pgsql-hackers |
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
pgsql-hackers by date: