Closed dur-randir closed 9 years ago
Thank you very much for your feedback. It looks reasonable from the first glance. I'll review it a more detailed way ASAP and let you know my thought.
Hi, is there any progress for checking this issue?
@dur-randir, sorry for not replying for so long.
Right, there is a subtle problem in the code that might lead to deadlocks, good catch. However, it can be fixed by just changing the order of operations in the switch block.
1> CREATE TABLE t (id INT PRIMARY KEY, data TEXT NOT NULL);
1> CREATE INDEX t_data_idx ON t (data);
1> INSERT INTO t VALUES (1, 'aaa');
1> BEGIN;
1> SELECT * FROM t WHERE id = 1 FOR UPDATE;
The SELECT FOR UPDATE above puts an AccessShareLock on t_data_idx in the 1st session.
2> CREATE INDEX CONCURRENTLY pgcompact_index_XXX ON t (data);
2> BEGIN;
2> ALTER INDEX t_data_idx RENAME TO pgcompact_swap_index_XXX;
The 2nd session starts waiting for an AccessExclusiveLock on t_data_idx, but nothing blocks the 1st one, so we can continue with the INSERT.
1> INSERT INTO t VALUES (2, 'bbb');
1> END;
After that the 2nd session acquires the lock and is able to finish the name switch.
2> ALTER INDEX pgcompact_index_xxx RENAME TO t_data_idx;
2> ALTER INDEX pgcompact_swap_index_xxx RENAME TO pgcompact_index_xxx;
2> END;
Would you like to update the PR with this changes?
Closing the PR as the issue has been fixed in https://github.com/grayhemp/pgtoolkit/commit/aac145e679dffd5ef23a82aed4890bd091f54236.
Call to '_swap_index_names' can cause deadlocks in the following scenario (console one - application, console 2 - pgcompact):
1> CREATE TABLE t (id INT PRIMARY KEY, data TEXT NOT NULL); 1> CREATE INDEX t_data_idx ON t (data); 1> INSERT INTO t VALUES (1, 'aaa');
1> BEGIN; 1> SELECT * FROM t WHERE id = 1 FOR UPDATE;
2> CREATE INDEX pgcompact_index_XXX ON t (data); 2> BEGIN; 2> ALTER INDEX pgcompact_index_XXX RENAME TO pgcompact_swap_index_XXX; 2> ALTER INDEX t_data_idx RENAME TO pgcompact_index_XXX;
1> INSERT INTO t VALUES (2, 'bbb');
And now we got stuck. This happens on 9.2-9.4, where '_can_drop_index_concurrently' is defined. It's inappropriate for an administrative tool to restrict application behavior, so I suggest to remove '_swap_index_names' and use only '_get_rename_temp_index_query' that takes heavier locks, but is deadlock-free.