Hello, Josh! Your review looks a bit LLM-generated, but anyway - thanks for review! :) Especially because at least one point seems to be valid.
> We're leaving behind two invalid indexes now that the user has to figure > out how to drop in case of an error - that seems like it could be confusing > for the user. > Could we have some better way (error handler, background worker) try to > perform this cleanup automatically? > If not, we should at least tell the user clearly in the error message that > both > invalid indexes are left behind (i.e. "idx" and "idx_ccaux" in the example) Commit 0005 adds automatic dropping of auxiliary indexes when the original index is reindexed or dropped. Also, documentation reflects the ccaux index (similar to ccnew). > Docs are inconsistent or confusing about whether there's one or two indexes > left behind in case of error > - e.g. "command will fail but leave behind *an* invalid index and its > associated auxiliary index" > somewhat implying there is only one invalid index, and somehow the auxiliary > index is valid? Auxiliary index is never marked as valid; I'm not sure we need to highlight it here. Or do you have an idea how to rephrase? > Similarly, when the doc mentions e.g. "drop the index" - it's not necessarily > clear which index > we're talking about when there are two invalid indexes left behind that the > user will see with `\d` In one commit it says: "method in such cases is to drop these indexes and try again to perform". After 0005 "The auxiliary index (suffixed with <literal>_ccaux</literal>) will be automatically dropped when the main index is dropped". It seems clear to me, but feel free to provide your variant. > * It would be nice to guard against users trying arbitrary CREATE INDEX ... > USING stir(...) with a clear error It will fail with "Building STIR indexes is not supported". > One of the testcases (line 2478 of patch 0004) does `DELETE FROM > concur_reindex_tab4 WHERE c1 = 1;` > but the table `concur_reindex_tab4` looks like it has been dropped a few > lines above that? Hm, yep, I'll fix it. > The StirPageGetFreeSpace macro from patch 0002 reads > `StirPageGetMaxOffset(page)` > which seems like it could cause an unsafe read of opaque->maxoff if used on > the metapage But it was never used for the metapage. > A comment explains "No predicate evaluation is needed here" , i.e. we are > skipping predicate > evaluation in the validation scan step, assuming that the > auxiliary index contains only qualifying TIDs. Is this really bulletproof for > e.g. partial indexes which may > no longer satisfy the predicate at the time of the validation scan due to > conflicting HOT updates? Conflicting HOT updates are not possible because the catalog contains the new index definition from the start of the process. Or do you mean a different scenario? Best regards, Mikhail.
