> A couple of minor comments about this patch: > 1) Reading it, I am wondering if it would not be finally time to > switch to a macro to get a relation's persistence, something like > RelationGetPersistence in rel.h... Not related directly to this patch.
> 2) reindex_index has as new argument a relpersislence value for the
> new index. reindex_relation has differently a new set of flags to > enforce the relpersistence of all the underling indexes. Wouldn't it > be better for API consistency to pass directly a relpersistence value > through reindex_relation? In any case, the comment block of > reindex_relation does not contain a description of the new flags.
I did it because the ReindexDatabase build a list of oids to run reindex_relation for each item of the list. I can change the list to store the relpersistence also, but this can lead us to a concurrency trouble because if one session execute REINDEX DATABASE and other session run "ALTER TABLE name SET {LOGGED|UNLOGGED}" during the building of the list the reindex can lead us to a inconsistent state.
Fair point. I forgot this code path.
Except a typo with s/rebuilded/rebuilt/ in the patch, corrected in the patch attached with some extra comments bundled, it seems to be that this patch can be passed to a committer, so marking it so.
Btw, perhaps this diff should be pushed as a different patch as this is a rather different thing: - if (heapRelation->rd_rel->relpersistence == RELPERSISTENCE_UNLOGGED && + if (indexRelation->rd_rel->relpersistence == RELPERSISTENCE_UNLOGGED && !smgrexists(indexRelation->rd_smgr, INIT_FORKNUM)
As this is a correctness fix, it does not seem necessary to back-patch it: the parent relation always has the same persistence as its indexes.
