On Mon, Mar 09, 2020 at 08:04:27AM +0100, Julien Rouhaud wrote:
> On Mon, Mar 09, 2020 at 02:52:31PM +0900, Michael Paquier wrote:
>> For the index-level operation, issuing a WARNING is not consistent
>> with the existing practice to use an ERROR, which is more adapted as
>> the operation is done on a single index at a time.
>
> Agreed.
Thanks for checking the patch.
>> It would have been nice to test this stuff. However, this requires an
>> invalid toast index and we cannot create that except by canceling a
>> concurrent reindex, leading us back to the upthread discussion about
>> isolation tests, timeouts and fault injection :/
>
> Yes, unfortunately I don't see an acceptable way to add tests for that without
> some kind of fault injection, so this will have to wait :(
Let's discuss that separately.
I have also been reviewing the isolation test you have added upthread
about the dependency handling of invalid indexes, and one thing that
we cannot really do is attempting to do a reindex at index or
table-level with invalid toast indexes as this leads to unstable ERROR
or WARNING messages. But at least one thing we can do is to extend
the query you sent directly so as it exposes the toast relation name
filtered with regex_replace(). This gives us a stable output, and
this way the test makes sure that the query cancellation happened
after the dependencies are swapped, and not at build or validation
time (indisvalid got appended to the end of the output):
+pg_toast.pg_toast_<oid>_index_ccoldf
+pg_toast.pg_toast_<oid>_indext
Please feel free to see the attached for reference, that's not
something for commit in upstream, but I am going to keep that around
in my own plugin tree.
--
Michael