On Tue, 11 Jun 2024 at 10:58, Michail Nikolaev <michail.nikol...@gmail.com> wrote: > > Hello. > > I did the POC (1) of the method described in the previous email, and it looks > promising. > > It doesn't block the VACUUM, indexes are built about 30% faster (22 mins vs > 15 mins).
That's a nice improvement. > Additional index is lightweight and does not produce any WAL. That doesn't seem to be what I see in the current patchset: https://github.com/postgres/postgres/compare/master...michail-nikolaev:postgres:new_index_concurrently_approach#diff-cc3cb8968cf833c4b8498ad2c561c786099c910515c4bf397ba853ae60aa2bf7R311 > I'll continue the more stress testing for a while. Also, I need to > restructure the commits (my path was no direct) into some meaningful and > reviewable patches. While waiting for this, here are some initial comments on the github diffs: - I notice you've added a new argument to heapam_index_build_range_scan. I think this could just as well be implemented by reading the indexInfo->ii_Concurrent field, as the values should be equivalent, right? - In heapam_index_build_range_scan, it seems like you're popping the snapshot and registering a new one while holding a tuple from heap_getnext(), thus while holding a page lock. I'm not so sure that's OK, expecially when catalogs are also involved (specifically for expression indexes, where functions could potentially be updated or dropped if we re-create the visibility snapshot) - In heapam_index_build_range_scan, you pop the snapshot before the returned heaptuple is processed and passed to the index-provided callback. I think that's incorrect, as it'll change the visibility of the returned tuple before it's passed to the index's callback. I think the snapshot manipulation is best added at the end of the loop, if we add it at all in that function. - The snapshot reset interval is quite high, at 500ms. Why did you configure it that low, and didn't you make this configurable? - You seem to be using WAL in the STIR index, while it doesn't seem that relevant for the use case of auxiliary indexes that won't return any data and are only used on the primary. It would imply that the data is being sent to replicas and more data being written than strictly necessary, which to me seems wasteful. - The locking in stirinsert can probably be improved significantly if we use things like atomic operations on STIR pages. We'd need an exclusive lock only for page initialization, while share locks are enough if the page's data is modified without WAL. That should improve concurrent insert performance significantly, as it would further reduce the length of the exclusively locked hot path. Kind regards, Matthias van de Meent Neon (https://neon.tech)
