PATCH: Issue with set_indexsafe_procflags in ReindexRelationConcurrently
Hello!
While working on [1]/messages/by-id/CANtu0ogBOtd9ravu1CUbuZWgq6qvn1rny38PGKDPk9zzQPH8_A@mail.gmail.com, I have found a small issue with correctness
of set_indexsafe_procflags usage in ReindexRelationConcurrently introduced
in [2]https://github.com/postgres/postgres/commit/f9900df5f94936067e6fa24a9df609863eb08da2.
idx->safe = (indexRel->rd_indexprs == NIL && indexRel->rd_indpred == NIL);
It is always true because there are no RelationGetIndexExpressions
and RelationGetIndexPredicate before that check.
Two patches with reproducer + fix are attached.
The issue is simple, but I'll register this in commitfest just in case.
Best regards,
Mikhail.
[1]: /messages/by-id/CANtu0ogBOtd9ravu1CUbuZWgq6qvn1rny38PGKDPk9zzQPH8_A@mail.gmail.com
/messages/by-id/CANtu0ogBOtd9ravu1CUbuZWgq6qvn1rny38PGKDPk9zzQPH8_A@mail.gmail.com
[2]: https://github.com/postgres/postgres/commit/f9900df5f94936067e6fa24a9df609863eb08da2
https://github.com/postgres/postgres/commit/f9900df5f94936067e6fa24a9df609863eb08da2
Attachments:
v1-0001-specification-to-reproduce-issue-with-incorrect-u.patchtext/x-patch; charset=US-ASCII; name=v1-0001-specification-to-reproduce-issue-with-incorrect-u.patchDownload+38-2
v1-0002-Ensure-the-correct-determination-of-index-safety-.patchtext/x-patch; charset=US-ASCII; name=v1-0002-Ensure-the-correct-determination-of-index-safety-.patchDownload+2-3
The issue is simple, but I'll register this in commitfest just in case.
https://commitfest.postgresql.org/50/5243/
Show quoted text
On Fri, Sep 06, 2024 at 01:27:12PM +0200, Michail Nikolaev wrote:
While working on [1], I have found a small issue with correctness
of set_indexsafe_procflags usage in ReindexRelationConcurrently introduced
in [2].idx->safe = (indexRel->rd_indexprs == NIL && indexRel->rd_indpred == NIL);
It is always true because there are no RelationGetIndexExpressions
and RelationGetIndexPredicate before that check.Two patches with reproducer + fix are attached.
The issue is simple, but I'll register this in commitfest just in case.
Ugh. It means that we've always considered as "safe" concurrent
rebuilds of indexes that have expressions or predicates, but they're
not safe at all. Other concurrent jobs should wait for them.
Adding these two calls as you are suggesting is probably a good idea
anyway to force a correct setup of the flag. Will see about that.
I don't understand why an isolation test is required here if we were
to add a validity test, because you can cause a failure in the REINDEX
with a set of SQLs in a single session. I'm OK to add a test, perhaps
with a NOTICE set when the safe flag is true. Anyway, what you have
is more than enough to prove your point. Thanks for that.
--
Michael