Обсуждение: Revisiting {CREATE INDEX, REINDEX} CONCURRENTLY improvements
Hello, hackers! I think about revisiting (1) ({CREATE INDEX, REINDEX} CONCURRENTLY improvements) in some lighter way. Yes, a serious bug was (2) caused by this optimization and now it reverted. But what about a more safe idea in that direction: 1) add new horizon which ignores PROC_IN_SAFE_IC backends and standbys queries 2) use this horizon for settings LP_DEAD bit in indexes (excluding indexes being built of course) Index LP_DEAD hints are not used by standby in any way (they are just ignored), also heap scan done by index building does not use them as well. But, at the same time: 1) index scans will be much faster during index creation or standby reporting queries 2) indexes can keep them fit using different optimizations 3) less WAL due to a huge amount of full pages writes (which caused by tons of LP_DEAD in indexes) The patch seems more-less easy to implement. Does it worth being implemented? Or to scary? [1]: https://postgr.es/m/20210115133858.GA18931@alvherre.pgsql [2]: https://postgr.es/m/17485-396609c6925b982d%40postgresql.org
On Fri, 15 Dec 2023, 20:07 Michail Nikolaev, <michail.nikolaev@gmail.com> wrote:
Hello, hackers!
I think about revisiting (1) ({CREATE INDEX, REINDEX} CONCURRENTLY
improvements) in some lighter way.
Yes, a serious bug was (2) caused by this optimization and now it reverted.
But what about a more safe idea in that direction:
1) add new horizon which ignores PROC_IN_SAFE_IC backends and standbys queries
2) use this horizon for settings LP_DEAD bit in indexes (excluding
indexes being built of course)
Index LP_DEAD hints are not used by standby in any way (they are just
ignored), also heap scan done by index building does not use them as
well.
But, at the same time:
1) index scans will be much faster during index creation or standby
reporting queries
2) indexes can keep them fit using different optimizations
3) less WAL due to a huge amount of full pages writes (which caused by
tons of LP_DEAD in indexes)
The patch seems more-less easy to implement.
Does it worth being implemented? Or to scary?
I hihgly doubt this is worth the additional cognitive overhead of another liveness state, and I think there might be other issues with marking index tuples dead in indexes before the table tuple is dead that I can't think of right now.
I've thought about alternative solutions, too: how about getting a new snapshot every so often?
We don't really care about the liveness of the already-scanned data; the snapshots used for RIC are used only during the scan. C/RIC's relation's lock level means vacuum can't run to clean up dead line items, so as long as we only swap the backend's reported snapshot (thus xmin) while the scan is between pages we should be able to reduce the time C/RIC is the one backend holding back cleanup of old tuples.
Kind regards,
Matthias van de Meent
Neon (https://neon.tech)
Hello! > I've thought about alternative solutions, too: how about getting a new snapshot every so often? > We don't really care about the liveness of the already-scanned data; the snapshots used for RIC > are used only during the scan. C/RIC's relation's lock level means vacuum can't run to clean up > dead line items, so as long as we only swap the backend's reported snapshot (thus xmin) while > the scan is between pages we should be able to reduce the time C/RIC is the one backend > holding back cleanup of old tuples. Hm, it looks like an interesting idea! It may be more dangerous, but at least it feels much more elegant than an LP_DEAD-related way. Also, feels like we may apply this to both phases (first and the second scans). The original patch (1) was helping only to the second one (after call to set_indexsafe_procflags). But for the first scan we allowed to do so only for non-unique indexes because of: > * The reason for doing that is to avoid > * bogus unique-index failures due to concurrent UPDATEs (we might see > * different versions of the same row as being valid when we pass over them, > * if we used HeapTupleSatisfiesVacuum). This leaves us with an index that > * does not contain any tuples added to the table while we built the index. Also, (1) was limited to indexes without expressions and predicates (2) because such may execute queries to other tables (sic!). One possible solution is to add some checks to make sure no user-defined functions are used. But as far as I understand, it affects only CIC for now and does not affect the ability to use the proposed technique (updating snapshot time to time). However, I think we need some more-less formal proof it is safe - it is really challenging to keep all the possible cases in the head. I’ll try to do something here. Another possible issue may be caused by the new locking pattern - we will be required to wait for all transaction started before the ending of the phase to exit. [1]: https://postgr.es/m/20210115133858.GA18931@alvherre.pgsql [2]: https://www.postgresql.org/message-id/flat/CAAaqYe_tq_Mtd9tdeGDsgQh%2BwMvouithAmcOXvCbLaH2PPGHvA%40mail.gmail.com#cbe3997b75c189c3713f243e25121c20
On Sun, 17 Dec 2023, 21:14 Michail Nikolaev, <michail.nikolaev@gmail.com> wrote: > > Hello! > > > I've thought about alternative solutions, too: how about getting a new snapshot every so often? > > We don't really care about the liveness of the already-scanned data; the snapshots used for RIC > > are used only during the scan. C/RIC's relation's lock level means vacuum can't run to clean up > > dead line items, so as long as we only swap the backend's reported snapshot (thus xmin) while > > the scan is between pages we should be able to reduce the time C/RIC is the one backend > > holding back cleanup of old tuples. > > Hm, it looks like an interesting idea! It may be more dangerous, but > at least it feels much more elegant than an LP_DEAD-related way. > Also, feels like we may apply this to both phases (first and the second scans). > The original patch (1) was helping only to the second one (after call > to set_indexsafe_procflags). > > But for the first scan we allowed to do so only for non-unique indexes > because of: > > > * The reason for doing that is to avoid > > * bogus unique-index failures due to concurrent UPDATEs (we might see > > * different versions of the same row as being valid when we pass over them, > > * if we used HeapTupleSatisfiesVacuum). This leaves us with an index that > > * does not contain any tuples added to the table while we built the index. Yes, for that we'd need an extra scan of the index that validates uniqueness. I think there was a proposal (though it may only have been an idea) some time ago, about turning existing non-unique indexes into unique ones by validating the data. Such a system would likely be very useful to enable this optimization. > Also, (1) was limited to indexes without expressions and predicates > (2) because such may execute queries to other tables (sic!). Note that the use of such expressions would be a violation of the function's definition; it would depend on data from other tables which makes the function behave like a STABLE function, as opposed to the IMMUTABLE that is required for index expressions. So, I don't think we should specially care about being correct for incorrectly marked function definitions. > One possible solution is to add some checks to make sure no > user-defined functions are used. > But as far as I understand, it affects only CIC for now and does not > affect the ability to use the proposed technique (updating snapshot > time to time). > > However, I think we need some more-less formal proof it is safe - it > is really challenging to keep all the possible cases in the head. I’ll > try to do something here. I just realised there is one issue with this design: We can't cheaply reset the snapshot during the second table scan: It is critically important that the second scan of R/CIC uses an index contents summary (made with index_bulk_delete) that was created while the current snapshot was already registered. If we didn't do that, the following would occur: 1. The index is marked ready for inserts from concurrent backends, but not yet ready for queries. 2. We get the bulkdelete data 3. A concurrent backend inserts a new tuple T on heap page P, inserts it into the index, and commits. This tuple is not in the summary, but has been inserted into the index. 4. R/CIC resets the snapshot, making T visible. 5. R/CIC scans page P, finds that tuple T has to be indexed but is not present in the summary, thus inserts that tuple into the index (which already had it inserted at 3) This thus would be a logic bug, as indexes assume at-most-once semantics for index tuple insertion; duplicate insertion are an error. So, the "reset the snapshot every so often" trick cannot be applied in phase 3 (the rescan), or we'd have to do an index_bulk_delete call every time we reset the snapshot. Rescanning might be worth the cost (e.g. when using BRIN), but that is very unlikely. Alternatively, we'd need to find another way to prevent us from inserting these duplicate entries - maybe by storing the scan's data in a buffer to later load into the index after another index_bulk_delete()? Counterpoint: for BRIN indexes that'd likely require a buffer much larger than the result index would be. Either way, for the first scan (i.e. phase 2 "build new indexes") this is not an issue: we don't care about what transaction adds/deletes tuples at that point. For all we know, all tuples of the table may be deleted concurrently before we even allow concurrent backends to start inserting tuples, and the algorithm would still work as it does right now. > Another possible issue may be caused by the new locking pattern - we > will be required to wait for all transaction started before the ending > of the phase to exit. What do you mean by "new locking pattern"? We already keep an ShareUpdateExclusiveLock on every heap table we're accessing during R/CIC, and that should already prevent any concurrent VACUUM operations, right? Kind regards, Matthias van de Meent Neon (https://neon.tech)
Hello! > Also, feels like we may apply this to both phases (first and the second scans). > The original patch (1) was helping only to the second one (after call > to set_indexsafe_procflags). Oops, I was wrong here. The original version of the patch was also applied to both phases. > Note that the use of such expressions would be a violation of the > function's definition; it would depend on data from other tables which > makes the function behave like a STABLE function, as opposed to the > IMMUTABLE that is required for index expressions. So, I don't think we > should specially care about being correct for incorrectly marked > function definitions. Yes, but such cases could probably cause crashes also... So, I think it is better to check them for custom functions. But I still not sure - if such limitations still required for proposed optimization or not. > I just realised there is one issue with this design: We can't cheaply > reset the snapshot during the second table scan: > It is critically important that the second scan of R/CIC uses an index > contents summary (made with index_bulk_delete) that was created while > the current snapshot was already registered. > So, the "reset the snapshot every so often" trick cannot be applied in > phase 3 (the rescan), or we'd have to do an index_bulk_delete call > every time we reset the snapshot. Rescanning might be worth the cost > (e.g. when using BRIN), but that is very unlikely. Hm, I think it is still possible. We could just manually recheck the tuples we see to the snapshot currently used for the scan. If an "old" snapshot can see the tuple also (HeapTupleSatisfiesHistoricMVCC) then search for it in the index summary. > What do you mean by "new locking pattern"? We already keep an > ShareUpdateExclusiveLock on every heap table we're accessing during > R/CIC, and that should already prevent any concurrent VACUUM > operations, right? I was thinking not about "classical" locking, but about waiting for other backends by WaitForLockers(heaplocktag, ShareLock, true). But I think everything should be fine. Best regards, Michail.
On Wed, 20 Dec 2023 at 10:56, Michail Nikolaev <michail.nikolaev@gmail.com> wrote: > > Note that the use of such expressions would be a violation of the > > function's definition; it would depend on data from other tables which > > makes the function behave like a STABLE function, as opposed to the > > IMMUTABLE that is required for index expressions. So, I don't think we > > should specially care about being correct for incorrectly marked > > function definitions. > > Yes, but such cases could probably cause crashes also... > So, I think it is better to check them for custom functions. But I > still not sure - > if such limitations still required for proposed optimization or not. I think contents could be inconsistent, but not more inconsistent than if the index was filled across multiple transactions using inserts. Either way I don't see it breaking more things that are not already broken in that way in other places - at most it will introduce another path that exposes the broken state caused by mislabeled functions. > > I just realised there is one issue with this design: We can't cheaply > > reset the snapshot during the second table scan: > > It is critically important that the second scan of R/CIC uses an index > > contents summary (made with index_bulk_delete) that was created while > > the current snapshot was already registered. > > > So, the "reset the snapshot every so often" trick cannot be applied in > > phase 3 (the rescan), or we'd have to do an index_bulk_delete call > > every time we reset the snapshot. Rescanning might be worth the cost > > (e.g. when using BRIN), but that is very unlikely. > > Hm, I think it is still possible. We could just manually recheck the > tuples we see > to the snapshot currently used for the scan. If an "old" snapshot can see > the tuple also (HeapTupleSatisfiesHistoricMVCC) then search for it in the > index summary. That's an interesting method. How would this deal with tuples not visible to the old snapshot? Presumably we can assume they're newer than that snapshot (the old snapshot didn't have it, but the new one does, so it's committed after the old snapshot, making them newer), so that backend must have inserted it into the index already, right? > HeapTupleSatisfiesHistoricMVCC That function has this comment marker: "Only usable on tuples from catalog tables!" Is that correct even for this? Should this deal with any potential XID wraparound, too? How does this behave when the newly inserted tuple's xmin gets frozen? This would be allowed to happen during heap page pruning, afaik - no rules that I know of which are against that - but it would create issues where normal snapshot visibility rules would indicate it visible to both snapshots regardless of whether it actually was visible to the older snapshot when that snapshot was created... Either way, "Historic snapshot" isn't something I've worked with before, so that goes onto my "figure out how it works" pile. Kind regards, Matthias van de Meent Neon (https://neon.tech)
Hello! > How would this deal with tuples not visible to the old snapshot? > Presumably we can assume they're newer than that snapshot (the old > snapshot didn't have it, but the new one does, so it's committed after > the old snapshot, making them newer), so that backend must have > inserted it into the index already, right? Yes, exactly. >> HeapTupleSatisfiesHistoricMVCC > That function has this comment marker: > "Only usable on tuples from catalog tables!" > Is that correct even for this? Yeah, we just need HeapTupleSatisfiesVisibility (which calls HeapTupleSatisfiesMVCC) instead. > Should this deal with any potential XID wraparound, too? Yeah, looks like we should care about such case somehow. Possible options here: 1) Skip vac_truncate_clog while CIC is running. In fact, I think it's not that much worse than the current state - datfrozenxid is still updated in the catalog and will be considered the next time vac_update_datfrozenxid is called (the next VACCUM on any table). 2) Delay vac_truncate_clog while CIC is running. In such a case, if it was skipped, we will need to re-run it using the index builds backend later. 3) Wait for 64-bit xids :) 4) Any ideas? In addition, for the first and second options, we need logic to cancel the second phase in the case of ForceTransactionIdLimitUpdate. But maybe I'm missing something and the tuples may be frozen, ignoring the set datfrozenxid values (over some horizon calculated at runtime based on the xmin backends). > How does this behave when the newly inserted tuple's xmin gets frozen? > This would be allowed to happen during heap page pruning, afaik - no > rules that I know of which are against that - but it would create > issues where normal snapshot visibility rules would indicate it > visible to both snapshots regardless of whether it actually was > visible to the older snapshot when that snapshot was created... Yes, good catch. Assuming we have somehow prevented vac_truncate_clog from occurring during CIC, we can leave frozen and potentially frozen (xmin<frozenXID) for the second phase. So, first phase processing items: * not frozen * xmin>frozenXID (may not be frozen) * visible by snapshot second phase: * frozen * xmin>frozenXID (may be frozen) * not in the index summary * visible by "old" snapshot You might also think – why is the first stage needed at all? Just use batch processing during initial index building? Best regards, Mikhail.
> Yes, good catch. > Assuming we have somehow prevented vac_truncate_clog from occurring > during CIC, we can leave frozen and potentially frozen > (xmin<frozenXID) for the second phase. Just realized that we can leave this for the first stage to improve efficiency. Since the ID is locked, anything that can be frozen will be visible in the first stage.
Hello. Realized my last idea is invalid (because tuples are frozen by using dynamically calculated horizon) - so, don't waste your time on it :) Need to think a little bit more here. Thanks, Mikhail.
Hello! It seems like the idea of "old" snapshot is still a valid one. > Should this deal with any potential XID wraparound, too? As far as I understand in our case, we are not affected by this in any way. Vacuum in our table is not possible because of locking, so, nothing may be frozen (see below). In the case of super long index building, transactional limits will stop new connections using current regular infrastructure because it is based on relation data (but not actual xmin of backends). > How does this behave when the newly inserted tuple's xmin gets frozen? > This would be allowed to happen during heap page pruning, afaik - no > rules that I know of which are against that - but it would create > issues where normal snapshot visibility rules would indicate it > visible to both snapshots regardless of whether it actually was > visible to the older snapshot when that snapshot was created... As I can see, heap_page_prune never freezes any tuples. In the case of regular vacuum, it used this way: call heap_page_prune and then call heap_prepare_freeze_tuple and then heap_freeze_execute_prepared. Merry Christmas, Mikhail.
On Mon, 25 Dec 2023 at 15:12, Michail Nikolaev <michail.nikolaev@gmail.com> wrote: > > Hello! > > It seems like the idea of "old" snapshot is still a valid one. > > > Should this deal with any potential XID wraparound, too? > > As far as I understand in our case, we are not affected by this in any way. > Vacuum in our table is not possible because of locking, so, nothing > may be frozen (see below). > In the case of super long index building, transactional limits will > stop new connections using current > regular infrastructure because it is based on relation data (but not > actual xmin of backends). > > > How does this behave when the newly inserted tuple's xmin gets frozen? > > This would be allowed to happen during heap page pruning, afaik - no > > rules that I know of which are against that - but it would create > > issues where normal snapshot visibility rules would indicate it > > visible to both snapshots regardless of whether it actually was > > visible to the older snapshot when that snapshot was created... > > As I can see, heap_page_prune never freezes any tuples. > In the case of regular vacuum, it used this way: call heap_page_prune > and then call heap_prepare_freeze_tuple and then > heap_freeze_execute_prepared. Correct, but there are changes being discussed where we would freeze tuples during pruning as well [0], which would invalidate that implementation detail. And, if I had to choose between improved opportunistic freezing and improved R/CIC, I'd probably choose improved freezing over R/CIC. As an alternative, we _could_ keep track of concurrent index inserts using a dummy index (with the same predicate) which only holds the TIDs of the inserted tuples. We'd keep it as an empty index in phase 1, and every time we reset the visibility snapshot we now only need to scan that index to know what tuples were concurrently inserted. This should have a significantly lower IO overhead than repeated full index bulkdelete scans for the new index in the second table scan phase of R/CIC. However, in a worst case it could still require another O(tablesize) of storage. Kind regards, Matthias van de Meent Neon (https://neon.tech) [0] https://www.postgresql.org/message-id/CAAKRu_a+g2oe6aHJCbibFtNFiy2aib4E31X9QYJ_qKjxZmZQEg@mail.gmail.com
Hello! > Correct, but there are changes being discussed where we would freeze > tuples during pruning as well [0], which would invalidate that > implementation detail. And, if I had to choose between improved > opportunistic freezing and improved R/CIC, I'd probably choose > improved freezing over R/CIC. As another option, we could extract a dedicated horizon value for an opportunistic freezing. And use some flags in R/CIC backend to keep it at the required value. Best regards, Michail.
Hello, Melanie! Sorry to interrupt you, just a quick question. > Correct, but there are changes being discussed where we would freeze > tuples during pruning as well [0], which would invalidate that > implementation detail. And, if I had to choose between improved > opportunistic freezing and improved R/CIC, I'd probably choose > improved freezing over R/CIC. Do you have any patches\threads related to that refactoring (opportunistic freezing of tuples during pruning) [0]? This may affect the idea of the current thread (latest version of it mostly in [1]) - it may be required to disable such a feature for particular relation temporary or affect horizon used for pruning (without holding xmin). Just no sure - is it reasonable to start coding right now, or wait for some prune-freeze-related patch first? [0] https://www.postgresql.org/message-id/CAAKRu_a+g2oe6aHJCbibFtNFiy2aib4E31X9QYJ_qKjxZmZQEg@mail.gmail.com [1] https://www.postgresql.org/message-id/flat/CANtu0ojRX%3DosoiXL9JJG6g6qOowXVbVYX%2BmDsN%2B2jmFVe%3DeG7w%40mail.gmail.com#a8ff53f23d0fc7edabd446b4d634e7b5
> > > I just realised there is one issue with this design: We can't cheaply > > > reset the snapshot during the second table scan: > > > It is critically important that the second scan of R/CIC uses an index > > > contents summary (made with index_bulk_delete) that was created while > > > the current snapshot was already registered. > > > > > So, the "reset the snapshot every so often" trick cannot be applied in > > > phase 3 (the rescan), or we'd have to do an index_bulk_delete call > > > every time we reset the snapshot. Rescanning might be worth the cost > > > (e.g. when using BRIN), but that is very unlikely. > > > > Hm, I think it is still possible. We could just manually recheck the > > tuples we see > > to the snapshot currently used for the scan. If an "old" snapshot can see > > the tuple also (HeapTupleSatisfiesHistoricMVCC) then search for it in the > > index summary. > That's an interesting method. > > How would this deal with tuples not visible to the old snapshot? > Presumably we can assume they're newer than that snapshot (the old > snapshot didn't have it, but the new one does, so it's committed after > the old snapshot, making them newer), so that backend must have > inserted it into the index already, right? I made a draft of the patch and this idea is not working. The problem is generally the same: * reference snapshot sees tuple X * reference snapshot is used to create index summary (but there is no tuple X in the index summary) * tuple X is updated to Y creating a HOT-chain * we started scan with new temporary snapshot (it sees Y, X is too old for it) * tuple X is pruned from HOT-chain because it is not protected by any snapshot * we see tuple Y in the scan with temporary snapshot * it is not in the index summary - so, we need to check if reference snapshot can see it * there is no way to understand if the reference snapshot was able to see tuple X - because we need the full HOT chain (with X tuple) for that Best regards, Michail.
On Thu, 1 Feb 2024, 17:06 Michail Nikolaev, <michail.nikolaev@gmail.com> wrote: > > > > > I just realised there is one issue with this design: We can't cheaply > > > > reset the snapshot during the second table scan: > > > > It is critically important that the second scan of R/CIC uses an index > > > > contents summary (made with index_bulk_delete) that was created while > > > > the current snapshot was already registered. I think the best way for this to work would be an index method that exclusively stores TIDs, and of which we can quickly determine new tuples, too. I was thinking about something like GIN's format, but using (generation number, tid) instead of ([colno, colvalue], tid) as key data for the internal trees, and would be unlogged (because the data wouldn't have to survive a crash). Then we could do something like this for the second table scan phase: 0. index->indisready is set [...] 1. Empty the "changelog index", resetting storage and the generation number. 2. Take index contents snapshot of new index, store this. 3. Loop until completion: 4a. Take visibility snapshot 4b. Update generation number of the changelog index, store this. 4c. Take index snapshot of "changelog index" for data up to the current stored generation number. Not including, because we only need to scan that part of the index that were added before we created our visibility snapshot, i.e. TIDs labeled with generation numbers between the previous iteration's generation number (incl.) and this iteration's generation (excl.). 4d. Combine the current index snapshot with that of the "changelog" index, and save this. Note that this needs to take care to remove duplicates. 4e. Scan segment of table (using the combined index snapshot) until we need to update our visibility snapshot or have scanned the whole table. This should give similar, if not the same, behavour as that which we have when we RIC a table with several small indexes, without requiring us to scan a full index of data several times. Attemp on proving this approach's correctness: In phase 3, after each step 4b: All matching tuples of the table that are in the visibility snapshot: * Were created before scan 1's snapshot, thus in the new index's snapshot, or * Were created after scan 1's snapshot but before index->indisready, thus not in the new index's snapshot, nor in the changelog index, or * Were created after the index was set as indisready, and committed before the previous iteration's visibility snapshot, thus in the combined index snapshot, or * Were created after the index was set as indisready, after the previous visibility snapshot was taken, but before the current visibility snapshot was taken, and thus definitely included in the changelog index. Because we hold a snapshot, no data in the table that we should see is removed, so we don't have a chance of broken HOT chains. Kind regards, Matthias van de Meent Neon (https://neon.tech)
Hello! > I think the best way for this to work would be an index method that > exclusively stores TIDs, and of which we can quickly determine new > tuples, too. I was thinking about something like GIN's format, but > using (generation number, tid) instead of ([colno, colvalue], tid) as > key data for the internal trees, and would be unlogged (because the > data wouldn't have to survive a crash) Yeah, this seems to be a reasonable approach, but there are some doubts related to it - it needs new index type as well as unlogged indexes to be introduced - this may make the patch too invasive to be merged. Also, some way to remove the index from the catalog in case of a crash may be required. A few more thoughts: * it is possible to go without generation number - we may provide a way to do some kind of fast index lookup (by TID) directly during the second table scan phase. * one more option is to maintain a Tuplesorts (instead of an index) with TIDs as changelog and merge with index snapshot after taking a new visibility snapshot. But it is not clear how to share the same Tuplesort with multiple inserting backends. * crazy idea - what is about to do the scan in the index we are building? We have tuple, so, we have all the data indexed in the index. We may try to do an index scan using that data to get all tuples and find the one with our TID :) Yes, in some cases it may be too bad because of the huge amount of TIDs we need to scan + also btree copies whole page despite we need single item. But some additional index method may help - feels like something related to uniqueness (but it is only in btree anyway). Thanks, Mikhail.
One more idea - is just forbid HOT prune while the second phase is running. It is not possible anyway currently because of snapshot held. Possible enhancements: * we may apply restriction only to particular tables * we may apply restrictions only to part of the tables (not yet scanned by R/CICs). Yes, it is not an elegant solution, limited, not reliable in terms of architecture, but a simple one.
On Wed, 21 Feb 2024 at 00:33, Michail Nikolaev <michail.nikolaev@gmail.com> wrote: > > Hello! > > I think the best way for this to work would be an index method that > > exclusively stores TIDs, and of which we can quickly determine new > > tuples, too. I was thinking about something like GIN's format, but > > using (generation number, tid) instead of ([colno, colvalue], tid) as > > key data for the internal trees, and would be unlogged (because the > > data wouldn't have to survive a crash) > > Yeah, this seems to be a reasonable approach, but there are some > doubts related to it - it needs new index type as well as unlogged > indexes to be introduced - this may make the patch too invasive to be > merged. I suppose so, though persistence is usually just to keep things correct in case of crashes, and this "index" is only there to support processes that don't expect to survive crashes. > Also, some way to remove the index from the catalog in case of > a crash may be required. That's less of an issue though, we already accept that a crash during CIC/RIC leaves unusable indexes around, so "needs more cleanup" is not exactly a blocker. > A few more thoughts: > * it is possible to go without generation number - we may provide a > way to do some kind of fast index lookup (by TID) directly during the > second table scan phase. While possible, I don't think this would be more performant than the combination approach, at the cost of potentially much more random IO when the table is aggressively being updated. > * one more option is to maintain a Tuplesorts (instead of an index) > with TIDs as changelog and merge with index snapshot after taking a > new visibility snapshot. But it is not clear how to share the same > Tuplesort with multiple inserting backends. Tuplesort requires the leader process to wait for concurrent backends to finish their sort before it can start consuming their runs. This would make it a very bad alternative to the "changelog index" as the CIC process would require on-demand actions from concurrent backends (flush of sort state). I'm not convinced that's somehow easier. > * crazy idea - what is about to do the scan in the index we are > building? We have tuple, so, we have all the data indexed in the > index. We may try to do an index scan using that data to get all > tuples and find the one with our TID :) We can't rely on that, because we have no guarantee we can find the tuple quickly enough. Equality-based indexing is very much optional, and so are TID-based checks (outside the current vacuum-related APIs), so finding one TID can (and probably will) take O(indexsize) when the tuple is not in the index, which is one reason for ambulkdelete() to exist. Kind regards, Matthias van de Meent
On Wed, 21 Feb 2024 at 09:35, Michail Nikolaev <michail.nikolaev@gmail.com> wrote: > > One more idea - is just forbid HOT prune while the second phase is > running. It is not possible anyway currently because of snapshot held. > > Possible enhancements: > * we may apply restriction only to particular tables > * we may apply restrictions only to part of the tables (not yet > scanned by R/CICs). > > Yes, it is not an elegant solution, limited, not reliable in terms of > architecture, but a simple one. How do you suppose this would work differently from a long-lived normal snapshot, which is how it works right now? Would it be exclusively for that relation? How would this be integrated with e.g. heap_page_prune_opt? Kind regards, Matthias van de Meent Neon (https://neon.tech)
Hi! > How do you suppose this would work differently from a long-lived > normal snapshot, which is how it works right now? Difference in the ability to take new visibility snapshot periodically during the second phase with rechecking visibility of tuple according to the "reference" snapshot (which is taken only once like now). It is the approach from (1) but with a workaround for the issues caused by heap_page_prune_opt. > Would it be exclusively for that relation? Yes, only for that affected relation. Other relations are unaffected. > How would this be integrated with e.g. heap_page_prune_opt? Probably by some flag in RelationData, but not sure here yet. If the idea looks sane, I could try to extend my POC - it should be not too hard, likely (I already have tests to make sure it is correct). (1): https://www.postgresql.org/message-id/flat/CANtu0oijWPRGRpaRR_OvT2R5YALzscvcOTFh-%3DuZKUpNJmuZtw%40mail.gmail.com#8141eb2ea177ff560ee713b3f20de404
On Wed, 21 Feb 2024 at 12:37, Michail Nikolaev <michail.nikolaev@gmail.com> wrote: > > Hi! > > > How do you suppose this would work differently from a long-lived > > normal snapshot, which is how it works right now? > > Difference in the ability to take new visibility snapshot periodically > during the second phase with rechecking visibility of tuple according > to the "reference" snapshot (which is taken only once like now). > It is the approach from (1) but with a workaround for the issues > caused by heap_page_prune_opt. > > > Would it be exclusively for that relation? > Yes, only for that affected relation. Other relations are unaffected. I suppose this could work. We'd also need to be very sure that the toast relation isn't cleaned up either: Even though that's currently DELETE+INSERT only and can't apply HOT, it would be an issue if we couldn't find the TOAST data of a deleted for everyone (but visible to us) tuple. Note that disabling cleanup for a relation will also disable cleanup of tuple versions in that table that are not used for the R/CIC snapshots, and that'd be an issue, too. > > How would this be integrated with e.g. heap_page_prune_opt? > Probably by some flag in RelationData, but not sure here yet. > > If the idea looks sane, I could try to extend my POC - it should be > not too hard, likely (I already have tests to make sure it is > correct). I'm not a fan of this approach. Changing visibility and cleanup semantics to only benefit R/CIC sounds like a pain to work with in essentially all visibility-related code. I'd much rather have to deal with another index AM, even if it takes more time: the changes in semantics will be limited to a new plug in the index AM system and a behaviour change in R/CIC, rather than behaviour that changes in all visibility-checking code. But regardless of second scan snapshots, I think we can worry about that part at a later moment: The first scan phase is usually the most expensive and takes the most time of all phases that hold snapshots, and in the above discussion we agreed that we can already reduce the time that a snapshot is held during that phase significantly. Sure, it isn't great that we have to scan the table again with only a single snapshot, but generally phase 2 doesn't have that much to do (except when BRIN indexes are involved) so this is likely less of an issue. And even if it is, we would still have reduced the number of long-lived snapshots by half. -Matthias
Hello! > I'm not a fan of this approach. Changing visibility and cleanup > semantics to only benefit R/CIC sounds like a pain to work with in > essentially all visibility-related code. I'd much rather have to deal > with another index AM, even if it takes more time: the changes in > semantics will be limited to a new plug in the index AM system and a > behaviour change in R/CIC, rather than behaviour that changes in all > visibility-checking code. Technically, this does not affect the visibility logic, only the clearing semantics. All visibility related code remains untouched. But yes, still an inelegant and a little strange-looking option. At the same time, perhaps it can be dressed in luxury somehow - for example, add as a first class citizen in ComputeXidHorizonsResult a list of blocks to clear some relations. > But regardless of second scan snapshots, I think we can worry about > that part at a later moment: The first scan phase is usually the most > expensive and takes the most time of all phases that hold snapshots, > and in the above discussion we agreed that we can already reduce the > time that a snapshot is held during that phase significantly. Sure, it > isn't great that we have to scan the table again with only a single > snapshot, but generally phase 2 doesn't have that much to do (except > when BRIN indexes are involved) so this is likely less of an issue. > And even if it is, we would still have reduced the number of > long-lived snapshots by half. Hmm, but it looks like we don't have the infrastructure to "update" xmin propagating to the horizon after the first snapshot in a transaction is taken. One option I know of is to reuse the d9d076222f5b94a85e0e318339cfc44b8f26022d (1) approach. But if this is the case, then there is no point in re-taking the snapshot again during the first phase - just apply this "if" only for the first phase - and you're done. Do you know any less-hacky way? Or is it a nice way to go? [1]: https://github.com/postgres/postgres/commit/d9d076222f5b94a85e0e318339cfc44b8f26022d#diff-8879f0173be303070ab7931db7c757c96796d84402640b9e386a4150ed97b179R1779-R1793
On Thu, 7 Mar 2024 at 19:37, Michail Nikolaev <michail.nikolaev@gmail.com> wrote: > > Hello! > > > I'm not a fan of this approach. Changing visibility and cleanup > > semantics to only benefit R/CIC sounds like a pain to work with in > > essentially all visibility-related code. I'd much rather have to deal > > with another index AM, even if it takes more time: the changes in > > semantics will be limited to a new plug in the index AM system and a > > behaviour change in R/CIC, rather than behaviour that changes in all > > visibility-checking code. > > Technically, this does not affect the visibility logic, only the > clearing semantics. > All visibility related code remains untouched. Yeah, correct. But it still needs to update the table relations' information after finishing creating the indexes, which I'd rather not have to do. > But yes, still an inelegant and a little strange-looking option. > > At the same time, perhaps it can be dressed in luxury > somehow - for example, add as a first class citizen in ComputeXidHorizonsResult > a list of blocks to clear some relations. Not sure what you mean here, but I don't think ComputeXidHorizonsResult should have anything to do with actual relations. > > But regardless of second scan snapshots, I think we can worry about > > that part at a later moment: The first scan phase is usually the most > > expensive and takes the most time of all phases that hold snapshots, > > and in the above discussion we agreed that we can already reduce the > > time that a snapshot is held during that phase significantly. Sure, it > > isn't great that we have to scan the table again with only a single > > snapshot, but generally phase 2 doesn't have that much to do (except > > when BRIN indexes are involved) so this is likely less of an issue. > > And even if it is, we would still have reduced the number of > > long-lived snapshots by half. > > Hmm, but it looks like we don't have the infrastructure to "update" xmin > propagating to the horizon after the first snapshot in a transaction is taken. We can just release the current snapshot, and get a new one, right? I mean, we don't actually use the transaction for much else than visibility during the first scan, and I don't think there is a need for an actual transaction ID until we're ready to mark the index entry with indisready. > One option I know of is to reuse the > d9d076222f5b94a85e0e318339cfc44b8f26022d (1) approach. > But if this is the case, then there is no point in re-taking the > snapshot again during the first > phase - just apply this "if" only for the first phase - and you're done. Not a fan of that, as it is too sensitive to abuse. Note that extensions will also have access to these tools, and I think we should build a system here that's not easy to break, rather than one that is. > Do you know any less-hacky way? Or is it a nice way to go? I suppose we could be resetting the snapshot every so often? Or use multiple successive TID range scans with a new snapshot each? Kind regards, Matthias van de Meent Neon (https://neon.tech)
Hello, Matthias!
> We can just release the current snapshot, and get a new one, right? I
> mean, we don't actually use the transaction for much else than
> visibility during the first scan, and I don't think there is a need
> for an actual transaction ID until we're ready to mark the index entry
> with indisready.
> I suppose we could be resetting the snapshot every so often? Or use
> multiple successive TID range scans with a new snapshot each?
It seems like it is not so easy in that case. Because we still need to hold catalog snapshot xmin, releasing the snapshot which used for the scan does not affect xmin propagated to the horizon.
That's why d9d076222f5b94a85e0e318339cfc44b8f26022d(1) affects only the data horizon, but not the catalog's one.
So, in such a situation, we may:
1) starts scan from scratch with some TID range multiple times. But such an approach feels too complex and error-prone for me.
2) split horizons propagated by `MyProc` to data-related xmin and catalog-related xmin. Like `xmin` and `catalogXmin`. We may just mark snapshots as affecting some of the horizons, or both. Such a change feels easy to be done but touches pretty core logic, so we need someone's approval for such a proposal, probably.
3) provide some less invasive (but less non-kludge) way: add some kind of process flag like `PROC_IN_SAFE_IC_XMIN` and function like `AdvanceIndexSafeXmin` which changes the way backend affect horizon calculation. In the case of `PROC_IN_SAFE_IC_XMIN` `ComputeXidHorizons` uses value from `proc->safeIcXmin` which is updated by `AdvanceIndexSafeXmin` while switching scan snapshots.
So, with option 2 or 3, we may avoid holding data horizon during the first phase scan by resetting the scan snapshot every so often (and, optionally, using `AdvanceIndexSafeXmin` in case of 3rd approach).
The same will be possible for the second phase (validate).
We may do the same "resetting the snapshot every so often" technique, but there is still the issue with the way we distinguish tuples which were missed by the first phase scan or were inserted into the index after the visibility snapshot was taken.
So, I see two options here:
1) approach with additional index with some custom AM proposed by you.
We may add some boolean flag to `Relation` about information of index building in progress (`indexisbuilding`).
It may be easily calculated using `(index->indisready && !index->indisvalid)`. For a more reliable solution, we also need to somehow check if backend/transaction building the index still in progress. Also, it is better to check if index is building concurrently using the "safe_index" way.
I think there is a non too complex and expensive way to do so, probably by addition of some flag to index catalog record.
Once we have such a flag, we may "legally" prohibit `heap_page_prune_opt` affecting the relation updating `GlobalVisHorizonKindForRel` like this:
if (rel != NULL && rel->rd_indexvalid && rel->rd_indexisbuilding)
return VISHORIZON_CATALOG;
So, in common it works this way:
* backend building the index affects catalog horizon as usual, but data horizon is regularly propagated forward during the scan. So, other relations are processed by vacuum and `heap_page_prune_opt` without any restrictions
* but our relation (with CIC in progress) accessed by `heap_page_prune_opt` (or any other vacuum-like mechanics) with catalog horizon to honor CIC work. Therefore, validating scan may be sure what none of the HOT-chain will be truncated. Even regular vacuum can't affect it (but yes, it can't be anyway because of relation locking).
As a result, we may easily distinguish tuples missed by first phase scan, just by testing them against reference snapshot (which used to take visibility snapshot).
So, for me, this approach feels non-kludge enough, safe and effective and the same time.
I have a prototype of this approach and looks like it works (I have a good test catching issues with index content for CIC).
What do you think about all this?
[1]: https://github.com/postgres/postgres/commit/d9d076222f5b94a85e0e318339cfc44b8f26022d#diff-8879f0173be303070ab7931db7c757c96796d84402640b9e386a4150ed97b179R1779-R1793
> We can just release the current snapshot, and get a new one, right? I
> mean, we don't actually use the transaction for much else than
> visibility during the first scan, and I don't think there is a need
> for an actual transaction ID until we're ready to mark the index entry
> with indisready.
> I suppose we could be resetting the snapshot every so often? Or use
> multiple successive TID range scans with a new snapshot each?
It seems like it is not so easy in that case. Because we still need to hold catalog snapshot xmin, releasing the snapshot which used for the scan does not affect xmin propagated to the horizon.
That's why d9d076222f5b94a85e0e318339cfc44b8f26022d(1) affects only the data horizon, but not the catalog's one.
So, in such a situation, we may:
1) starts scan from scratch with some TID range multiple times. But such an approach feels too complex and error-prone for me.
2) split horizons propagated by `MyProc` to data-related xmin and catalog-related xmin. Like `xmin` and `catalogXmin`. We may just mark snapshots as affecting some of the horizons, or both. Such a change feels easy to be done but touches pretty core logic, so we need someone's approval for such a proposal, probably.
3) provide some less invasive (but less non-kludge) way: add some kind of process flag like `PROC_IN_SAFE_IC_XMIN` and function like `AdvanceIndexSafeXmin` which changes the way backend affect horizon calculation. In the case of `PROC_IN_SAFE_IC_XMIN` `ComputeXidHorizons` uses value from `proc->safeIcXmin` which is updated by `AdvanceIndexSafeXmin` while switching scan snapshots.
So, with option 2 or 3, we may avoid holding data horizon during the first phase scan by resetting the scan snapshot every so often (and, optionally, using `AdvanceIndexSafeXmin` in case of 3rd approach).
The same will be possible for the second phase (validate).
We may do the same "resetting the snapshot every so often" technique, but there is still the issue with the way we distinguish tuples which were missed by the first phase scan or were inserted into the index after the visibility snapshot was taken.
So, I see two options here:
1) approach with additional index with some custom AM proposed by you.
It looks correct and reliable but feels complex to implement and maintain. Also, it negatively affects performance of table access (because of an additional index) and validation scan (because we need to merge additional index content with visibility snapshot).
2) one more tricky approach.
2) one more tricky approach.
We may add some boolean flag to `Relation` about information of index building in progress (`indexisbuilding`).
It may be easily calculated using `(index->indisready && !index->indisvalid)`. For a more reliable solution, we also need to somehow check if backend/transaction building the index still in progress. Also, it is better to check if index is building concurrently using the "safe_index" way.
I think there is a non too complex and expensive way to do so, probably by addition of some flag to index catalog record.
Once we have such a flag, we may "legally" prohibit `heap_page_prune_opt` affecting the relation updating `GlobalVisHorizonKindForRel` like this:
if (rel != NULL && rel->rd_indexvalid && rel->rd_indexisbuilding)
return VISHORIZON_CATALOG;
So, in common it works this way:
* backend building the index affects catalog horizon as usual, but data horizon is regularly propagated forward during the scan. So, other relations are processed by vacuum and `heap_page_prune_opt` without any restrictions
* but our relation (with CIC in progress) accessed by `heap_page_prune_opt` (or any other vacuum-like mechanics) with catalog horizon to honor CIC work. Therefore, validating scan may be sure what none of the HOT-chain will be truncated. Even regular vacuum can't affect it (but yes, it can't be anyway because of relation locking).
As a result, we may easily distinguish tuples missed by first phase scan, just by testing them against reference snapshot (which used to take visibility snapshot).
So, for me, this approach feels non-kludge enough, safe and effective and the same time.
I have a prototype of this approach and looks like it works (I have a good test catching issues with index content for CIC).
What do you think about all this?
[1]: https://github.com/postgres/postgres/commit/d9d076222f5b94a85e0e318339cfc44b8f26022d#diff-8879f0173be303070ab7931db7c757c96796d84402640b9e386a4150ed97b179R1779-R1793
Hello, Matthias!
I just realized there is a much simpler and safe way to deal with the problem.
So, d9d076222f5b94a85e0e318339cfc44b8f26022d(1) had a bug because the scan was not protected by a snapshot. At the same time, we want this snapshot to affect not all the relations, but only a subset of them. And there is already a proper way to achieve that - different types of visibility horizons!
So, to resolve the issue, we just need to create a separated horizon value for such situation as building an index concurrently.
For now, let's name it `VISHORIZON_BUILD_INDEX_CONCURRENTLY` for example. By default, its value is equal to `VISHORIZON_DATA`. But in some cases it "stops" propagating forward while concurrent index is building, like this:
h->create_index_concurrently_oldest_nonremovable =TransactionIdOlder(h->create_index_concurrently_oldest_nonremovable, xmin);
if (!(statusFlags & PROC_IN_SAFE_IC))
h->data_oldest_nonremovable = TransactionIdOlder(h->data_oldest_nonremovable, xmin);
The `PROC_IN_SAFE_IC` marks backend xmin as ignored by `VISHORIZON_DATA` but not by `VISHORIZON_BUILD_INDEX_CONCURRENTLY`.
After, we need to use appropriate horizon for relations which are processed by `PROC_IN_SAFE_IC` backends. There are a few ways to do it, we may start prototyping with `rd_indexisbuilding` from previous message:
static inline GlobalVisHorizonKind
GlobalVisHorizonKindForRel(Relation rel)
........
if (rel != NULL && rel->rd_indexvalid && rel->rd_indexisbuilding)
return VISHORIZON_BUILD_INDEX_CONCURRENTLY;
There are few more moments need to be considered:
* Does it move the horizon backwards?
It is allowed for the horizon to move backwards (like said in `ComputeXidHorizons`) but anyway - in that case the horizon for particular relations just starts to lag behind the horizon for other relations.
Invariant is like that: `VISHORIZON_BUILD_INDEX_CONCURRENTLY` <= `VISHORIZON_DATA` <= `VISHORIZON_CATALOG` <= `VISHORIZON_SHARED`.
* What is about old cached versions of `Relation` objects without `rd_indexisbuilding` yet set?
This is not a problem because once the backend registers a new index, it waits for all transactions without that knowledge to end (`WaitForLockers`). So, new ones will also get information about new horizon for that particular relation.
* What is about TOAST?
To keep TOAST horizon aligned with relation building the index, we may do the next thing (as first implementation iteration):
else if (rel != NULL && ((rel->rd_indexvalid && rel->rd_indexisbuilding) || IsToastRelation(rel)))
return VISHORIZON_BUILD_INDEX_CONCURRENTLY;
For the normal case, `VISHORIZON_BUILD_INDEX_CONCURRENTLY` is equal to `VISHORIZON_DATA` - nothing is changed at all. But while the concurrent index is building, the TOAST horizon is guaranteed to be aligned with its parent relation. And yes, it is better to find an easy way to affect only TOAST relations related to the relation with index building in progress.
New horizon adds some complexity, but not too much, in my opinion. I am pretty sure it is worth being done because the ability to rebuild indexes without performance degradation is an extremely useful feature.
I just realized there is a much simpler and safe way to deal with the problem.
So, d9d076222f5b94a85e0e318339cfc44b8f26022d(1) had a bug because the scan was not protected by a snapshot. At the same time, we want this snapshot to affect not all the relations, but only a subset of them. And there is already a proper way to achieve that - different types of visibility horizons!
So, to resolve the issue, we just need to create a separated horizon value for such situation as building an index concurrently.
For now, let's name it `VISHORIZON_BUILD_INDEX_CONCURRENTLY` for example. By default, its value is equal to `VISHORIZON_DATA`. But in some cases it "stops" propagating forward while concurrent index is building, like this:
h->create_index_concurrently_oldest_nonremovable =TransactionIdOlder(h->create_index_concurrently_oldest_nonremovable, xmin);
if (!(statusFlags & PROC_IN_SAFE_IC))
h->data_oldest_nonremovable = TransactionIdOlder(h->data_oldest_nonremovable, xmin);
The `PROC_IN_SAFE_IC` marks backend xmin as ignored by `VISHORIZON_DATA` but not by `VISHORIZON_BUILD_INDEX_CONCURRENTLY`.
After, we need to use appropriate horizon for relations which are processed by `PROC_IN_SAFE_IC` backends. There are a few ways to do it, we may start prototyping with `rd_indexisbuilding` from previous message:
static inline GlobalVisHorizonKind
GlobalVisHorizonKindForRel(Relation rel)
........
if (rel != NULL && rel->rd_indexvalid && rel->rd_indexisbuilding)
return VISHORIZON_BUILD_INDEX_CONCURRENTLY;
There are few more moments need to be considered:
* Does it move the horizon backwards?
It is allowed for the horizon to move backwards (like said in `ComputeXidHorizons`) but anyway - in that case the horizon for particular relations just starts to lag behind the horizon for other relations.
Invariant is like that: `VISHORIZON_BUILD_INDEX_CONCURRENTLY` <= `VISHORIZON_DATA` <= `VISHORIZON_CATALOG` <= `VISHORIZON_SHARED`.
* What is about old cached versions of `Relation` objects without `rd_indexisbuilding` yet set?
This is not a problem because once the backend registers a new index, it waits for all transactions without that knowledge to end (`WaitForLockers`). So, new ones will also get information about new horizon for that particular relation.
* What is about TOAST?
To keep TOAST horizon aligned with relation building the index, we may do the next thing (as first implementation iteration):
else if (rel != NULL && ((rel->rd_indexvalid && rel->rd_indexisbuilding) || IsToastRelation(rel)))
return VISHORIZON_BUILD_INDEX_CONCURRENTLY;
For the normal case, `VISHORIZON_BUILD_INDEX_CONCURRENTLY` is equal to `VISHORIZON_DATA` - nothing is changed at all. But while the concurrent index is building, the TOAST horizon is guaranteed to be aligned with its parent relation. And yes, it is better to find an easy way to affect only TOAST relations related to the relation with index building in progress.
New horizon adds some complexity, but not too much, in my opinion. I am pretty sure it is worth being done because the ability to rebuild indexes without performance degradation is an extremely useful feature.
Things to be improved:
* better way to track relations with concurrent indexes being built (with mechanics to understood what index build was failed)
* better way to affect TOAST tables only related to concurrent index build
* better naming
Patch prototype in attachment.
Also, maybe it is worth committing test separately - it was based on Andrey Borodin work (2). The test fails well in the case of incorrect implementation.
[1]: https://github.com/postgres/postgres/commit/d9d076222f5b94a85e0e318339cfc44b8f26022d#diff-8879f0173be303070ab7931db7c757c96796d84402640b9e386a4150ed97b179R1779-R1793
[2]: https://github.com/x4m/postgres_g/commit/d0651e7d0d14862d5a4dac076355
* better way to track relations with concurrent indexes being built (with mechanics to understood what index build was failed)
* better way to affect TOAST tables only related to concurrent index build
* better naming
Patch prototype in attachment.
Also, maybe it is worth committing test separately - it was based on Andrey Borodin work (2). The test fails well in the case of incorrect implementation.
[1]: https://github.com/postgres/postgres/commit/d9d076222f5b94a85e0e318339cfc44b8f26022d#diff-8879f0173be303070ab7931db7c757c96796d84402640b9e386a4150ed97b179R1779-R1793
[2]: https://github.com/x4m/postgres_g/commit/d0651e7d0d14862d5a4dac076355
Вложения
Hello, Matthias and others!
Updated WIP in attach.
Changes are:
* Renaming, now it feels better for me
* More reliable approach in `GlobalVisHorizonKindForRel` to make sure we have not missed `rd_safeindexconcurrentlybuilding` by calling `RelationGetIndexList` if required
* Optimization to avoid any additional `RelationGetIndexList` if zero of concurrently indexes are being built
* TOAST moved to TODO, since looks like it is out of scope - but not sure yet, need to dive dipper
Updated WIP in attach.
Changes are:
* Renaming, now it feels better for me
* More reliable approach in `GlobalVisHorizonKindForRel` to make sure we have not missed `rd_safeindexconcurrentlybuilding` by calling `RelationGetIndexList` if required
* Optimization to avoid any additional `RelationGetIndexList` if zero of concurrently indexes are being built
* TOAST moved to TODO, since looks like it is out of scope - but not sure yet, need to dive dipper
TODO:
* TOAST
* docs and comments
* make sure non-data tables are not affected
* Per-database scope of optimization
* Handle index building errors correctly in optimization code
* More tests: create index, multiple re-indexes, multiple tables
Thanks,
Michail.
Thanks,
Michail.
Вложения
Hi again!
Made an error in `GlobalVisHorizonKindForRel` logic, and it was caught by a new test.
Fixed version in attach.
Made an error in `GlobalVisHorizonKindForRel` logic, and it was caught by a new test.
Fixed version in attach.
Вложения
Hello, Matthias and others!
Realized new horizon was applied only during validation phase (once index is marked as ready).
Now it applied if index is not marked as valid yet.
Updated version in attach.
--------------------------------------------------
Realized new horizon was applied only during validation phase (once index is marked as ready).
Now it applied if index is not marked as valid yet.
Updated version in attach.
--------------------------------------------------
> I think the best way for this to work would be an index method that
> exclusively stores TIDs, and of which we can quickly determine new
> tuples, too. I was thinking about something like GIN's format, but
> using (generation number, tid) instead of ([colno, colvalue], tid) as
> key data for the internal trees, and would be unlogged (because the
> data wouldn't have to survive a crash). Then we could do something
> like this for the second table scan phase:
Regarding that approach to dealing with validation phase and resetting of snapshot:
I was thinking about it and realized: once we go for an additional index - we don't need the second heap scan at all!
We may do it this way:
* create target index, not marked as indisready yet
* create a temporary unlogged index with the same parameters to store tids (optionally with the indexes columns data, see below), marked as indisready (but not indisvalid)
* commit them both in a single transaction
* wait for other transaction to know about them and honor in HOT constraints and new inserts (for temporary index)
* now our temporary index is filled by the tuples inserted to the table
* start building out target index, resetting snapshot every so often (if it is "safe" index)
* finish target index building phase
* mark target index as indisready
* now, start validation of the index:
* take the reference snapshot
* take a visibility snapshot of the target index, sort it (as it done currently)
* take a visibility snapshot of our temporary index, sort it
* start merging loop using two synchronized cursors over both visibility snapshots
* if we encountered tid which is not present in target visibility snapshot
* insert it to target index
* if a temporary index contains the column's data - we may even avoid the tuple fetch
* if temporary index is tid-only - we fetch tuple from the heap, but as plus we are also skipping dead tuples from insertion to the new index (I think it is better option)
* commit everything, release reference snapshot
* wait for transactions older than reference snapshot (as it done currently)
* mark target index as indisvalid, drop temporary index
* done
So, pros:
* just a single heap scan
* snapshot is reset periodically
Cons:
* we need to maintain the additional index during the main building phase
* one more tuplesort
If the temporary index is unlogged, cheap to maintain (just append-only mechanics) this feels like a perfect tradeoff for me.
This approach will work perfectly with low amount of tuple inserts during the building phase. And looks like even in the worst case it still better than the current approach.
What do you think? Have I missed something?
Thanks,
Michail.
> exclusively stores TIDs, and of which we can quickly determine new
> tuples, too. I was thinking about something like GIN's format, but
> using (generation number, tid) instead of ([colno, colvalue], tid) as
> key data for the internal trees, and would be unlogged (because the
> data wouldn't have to survive a crash). Then we could do something
> like this for the second table scan phase:
Regarding that approach to dealing with validation phase and resetting of snapshot:
I was thinking about it and realized: once we go for an additional index - we don't need the second heap scan at all!
We may do it this way:
* create target index, not marked as indisready yet
* create a temporary unlogged index with the same parameters to store tids (optionally with the indexes columns data, see below), marked as indisready (but not indisvalid)
* commit them both in a single transaction
* wait for other transaction to know about them and honor in HOT constraints and new inserts (for temporary index)
* now our temporary index is filled by the tuples inserted to the table
* start building out target index, resetting snapshot every so often (if it is "safe" index)
* finish target index building phase
* mark target index as indisready
* now, start validation of the index:
* take the reference snapshot
* take a visibility snapshot of the target index, sort it (as it done currently)
* take a visibility snapshot of our temporary index, sort it
* start merging loop using two synchronized cursors over both visibility snapshots
* if we encountered tid which is not present in target visibility snapshot
* insert it to target index
* if a temporary index contains the column's data - we may even avoid the tuple fetch
* if temporary index is tid-only - we fetch tuple from the heap, but as plus we are also skipping dead tuples from insertion to the new index (I think it is better option)
* commit everything, release reference snapshot
* wait for transactions older than reference snapshot (as it done currently)
* mark target index as indisvalid, drop temporary index
* done
So, pros:
* just a single heap scan
* snapshot is reset periodically
Cons:
* we need to maintain the additional index during the main building phase
* one more tuplesort
If the temporary index is unlogged, cheap to maintain (just append-only mechanics) this feels like a perfect tradeoff for me.
This approach will work perfectly with low amount of tuple inserts during the building phase. And looks like even in the worst case it still better than the current approach.
What do you think? Have I missed something?
Thanks,
Michail.
Вложения
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). Additional index is lightweight and does not produce any WAL.
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.
On Tue, 11 Jun 2024 at 10:58, Michail Nikolaev <michail.nikolaev@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) intosome 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)
Hello, Matthias!
> While waiting for this, here are some initial comments on the github diffs:
Thanks for your review!
While stress testing the POC, I found some issues unrelated to the patch that need to be fixed first.
This is [1] and [2].
>> Additional index is lightweight and does not produce any WAL.
> That doesn't seem to be what I see in the current patchset:
Persistence is passed as parameter [3] and set to RELPERSISTENCE_UNLOGGED for auxiliary indexes [4].
> - 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?
Not always; currently, it is set by ResetSnapshotsAllowed[5].
We fall back to regular index build if there is a predicate or expression in the index (which should be considered "safe" according to [6]).
However, we may remove this check later.
Additionally, there is no sense in resetting the snapshot if we already have an xmin assigned to the backend for some reason.
> 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)
Yeah, good catch.
Initially, I implemented a different approach by extracting the catalog xmin to a separate horizon [7]. It might be better to return to this option.
> 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.
Yes, this needs to be fixed as well.
> The snapshot reset interval is quite high, at 500ms. Why did you
> configure it that low, and didn't you make this configurable?
It is just a random value for testing purposes.
I don't think there is a need to make it configurable.
Getting a new snapshot is a cheap operation now, so we can do it more often if required.
Internally, I was testing it with a 0ms interval.
> 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.
It just looks like an index with WAL, but as mentioned above, it is unlogged in actual usage.
> 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.
Hm, good idea. I'll check it later.
Best regards & thanks again,
Mikhail
[1]: https://www.postgresql.org/message-id/CANtu0ohHmYXsK5bxU9Thcq1FbELLAk0S2Zap0r8AnU3OTmcCOA%40mail.gmail.com
[2]: https://www.postgresql.org/message-id/CANtu0ojga8s9%2BJ89cAgLzn2e-bQgy3L0iQCKaCnTL%3Dppot%3Dqhw%40mail.gmail.com
[3]: https://github.com/postgres/postgres/compare/master...michail-nikolaev:postgres:new_index_concurrently_approach#diff-50abc48efcc362f0d3194aceba6969429f46fa1f07a119e555255545e6655933R93
[4]: https://github.com/michail-nikolaev/postgres/blob/e2698ca7c814a5fa5d4de8a170b7cae83034cade/src/backend/catalog/index.c#L1600
[5]: https://github.com/michail-nikolaev/postgres/blob/e2698ca7c814a5fa5d4de8a170b7cae83034cade/src/backend/catalog/index.c#L2657
[6]: https://github.com/michail-nikolaev/postgres/blob/e2698ca7c814a5fa5d4de8a170b7cae83034cade/src/backend/commands/indexcmds.c#L1129
[7]: https://github.com/postgres/postgres/commit/38b243d6cc7358a44cb1a865b919bf9633825b0c
> While waiting for this, here are some initial comments on the github diffs:
Thanks for your review!
While stress testing the POC, I found some issues unrelated to the patch that need to be fixed first.
This is [1] and [2].
>> Additional index is lightweight and does not produce any WAL.
> That doesn't seem to be what I see in the current patchset:
Persistence is passed as parameter [3] and set to RELPERSISTENCE_UNLOGGED for auxiliary indexes [4].
> - 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?
Not always; currently, it is set by ResetSnapshotsAllowed[5].
We fall back to regular index build if there is a predicate or expression in the index (which should be considered "safe" according to [6]).
However, we may remove this check later.
Additionally, there is no sense in resetting the snapshot if we already have an xmin assigned to the backend for some reason.
> 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)
Yeah, good catch.
Initially, I implemented a different approach by extracting the catalog xmin to a separate horizon [7]. It might be better to return to this option.
> 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.
Yes, this needs to be fixed as well.
> The snapshot reset interval is quite high, at 500ms. Why did you
> configure it that low, and didn't you make this configurable?
It is just a random value for testing purposes.
I don't think there is a need to make it configurable.
Getting a new snapshot is a cheap operation now, so we can do it more often if required.
Internally, I was testing it with a 0ms interval.
> 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.
It just looks like an index with WAL, but as mentioned above, it is unlogged in actual usage.
> 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.
Hm, good idea. I'll check it later.
Best regards & thanks again,
Mikhail
[1]: https://www.postgresql.org/message-id/CANtu0ohHmYXsK5bxU9Thcq1FbELLAk0S2Zap0r8AnU3OTmcCOA%40mail.gmail.com
[2]: https://www.postgresql.org/message-id/CANtu0ojga8s9%2BJ89cAgLzn2e-bQgy3L0iQCKaCnTL%3Dppot%3Dqhw%40mail.gmail.com
[3]: https://github.com/postgres/postgres/compare/master...michail-nikolaev:postgres:new_index_concurrently_approach#diff-50abc48efcc362f0d3194aceba6969429f46fa1f07a119e555255545e6655933R93
[4]: https://github.com/michail-nikolaev/postgres/blob/e2698ca7c814a5fa5d4de8a170b7cae83034cade/src/backend/catalog/index.c#L1600
[5]: https://github.com/michail-nikolaev/postgres/blob/e2698ca7c814a5fa5d4de8a170b7cae83034cade/src/backend/catalog/index.c#L2657
[6]: https://github.com/michail-nikolaev/postgres/blob/e2698ca7c814a5fa5d4de8a170b7cae83034cade/src/backend/commands/indexcmds.c#L1129
[7]: https://github.com/postgres/postgres/commit/38b243d6cc7358a44cb1a865b919bf9633825b0c