Hi, On 2019-05-01 19:41:24 -0400, Tom Lane wrote: > I wrote: > > Andres Freund <and...@anarazel.de> writes: > >> On 2019-05-01 10:06:03 -0700, Andres Freund wrote: > >>> I'm not sure this is the right short-term answer. Why isn't it, for now, > >>> sufficient to do what I suggested with RelationSetNewRelfilenode() not > >>> doing the CommandCounterIncrement(), and reindex_index() then doing the > >>> SetReindexProcessing() before a CommandCounterIncrement()? That's like > >>> ~10 line code change, and a few more with comments. > > > That looks like a hack to me... > > > The main thing I'm worried about right now is that I realized that > > our recovery from errors in this area is completely hosed, cf > > https://www.postgresql.org/message-id/4541.1556736...@sss.pgh.pa.us > > OK, so per the other thread, it seems like the error recovery problem > isn't going to affect this directly. However, I still don't like this > proposal much; the reason being that it's a rather fundamental change > in the API of RelationSetNewRelfilenode. This will certainly break > any external callers of that function --- and silently, too. > > Admittedly, there might not be any outside callers, but I don't really > like that assumption for something we're going to have to back-patch.
Couldn't we just address that by adding a new RelationSetNewRelfilenodeInternal() that's then wrapped by RelationSetNewRelfilenode() which just does RelationSetNewRelfilenodeInternal();CCI();? Doesn't have to be ...Internal(), could also be RelationBeginSetNewRelfilenode() or such. I'm not sure why you think using CCI() for this purpose is a hack? To me the ability to have catalog changes only take effect when they're all done, and the system is ready for them, is one of the core purposes of the infrastructure? > The solution I'm thinking of should have much more localized effects, > basically just in reindex_index and RelationSetNewRelfilenode, which is > why I like it better. Well, as I said before, I think hiding the to-be-rebuilt index from the list of indexes is dangerous too - if somebody added an actual CatalogUpdate/Insert (rather than inplace_update) anywhere along the index_build() path, we'd not get an assertion failure anymore, but just an index without the new entry. And given the fragility with HOT hiding that a lot of the time, that seems dangerous to me. Greetings, Andres Freund
