Fix error printout on correct security label #3799
Conversation
mkindahl
force-pushed
the
fix-seclabel-error
branch
from
401d4eb
to
322ccd8
Compare
mkindahl
force-pushed
the
fix-seclabel-error
branch
from
322ccd8
to
80ccffb
Compare
Codecov Report
@@ Coverage Diff @@
## master #3799 +/- ##
==========================================
- Coverage 90.56% 90.49% -0.08%
==========================================
Files 213 213
Lines 37771 37773 +2
==========================================
- Hits 34209 34182 -27
- Misses 3562 3591 +29
Continue to review full report at Codecov.
|
|
Looks OK but it would be nice to add a few more test cases to improve the code coverage. |
src/loader/loader.c
Outdated
| * containing 4 dashes, and with groups of digits of the size 8-4-4-4-12. | ||
| */ | ||
| static bool | ||
| is_uuid(const char *str) |
There was a problem hiding this comment.
Maybe it makes sense to reuse PostgreSQL types/functions for such validation
There was a problem hiding this comment.
I was thinking the same thing. Why not just call uuid_in on this string? If necessary, capture and rewrite the error.
There was a problem hiding this comment.
The function is uuid_in, which generate an error if it fails parsing, and capturing the error and replacing it with a different one was more complicated than writing this function. If you feel strongly about it, I can replace it.
There was a problem hiding this comment.
Switched to use uuid_in, as suggested, after our discussion.
mkindahl
force-pushed
the
fix-seclabel-error
branch
from
80ccffb
to
5d22a56
Compare
src/loader/loader.c
Outdated
| ereport(ERROR, | ||
| (errmsg("TimescaleDB label is for internal use only"), | ||
| errdetail("Security label is \"%s\".", stmt->label), | ||
| errhint("Security label has to be of format \"dist_uuid:<UUID>\"."))); |
There was a problem hiding this comment.
I don't think this hint is appropriate since dist_uuid is not something a user should set. If this error happens, then either the dist_uuid is corrupted or the code mistook another UUID for the dist_uuid. In neither case should the user try to change the format to fix this issue or add this label him/herself.
I would probably just get rid of the hint.
There was a problem hiding this comment.
Understanding why something is a problem and tracking down the issue is just as important as being able to fix it. This hint is intended for users to understand what is wrong, and not so much for being able to repair it, so I think there is value in having this hint for debugging purposes.
mkindahl
force-pushed
the
fix-seclabel-error
branch
from
5d22a56
to
3225d88
Compare
Added test cases to cover the cases highlighted by coverity. |
| ereport(ERROR, | ||
| (errmsg("TimescaleDB label is for internal use only"), | ||
| errdetail("Security label is \"%s\".", label), | ||
| errhint("Security label has to be of format \"dist_uuid:<UUID>\"."))); |
There was a problem hiding this comment.
My previous comment about dropping the hint still applies.
There was a problem hiding this comment.
I prefer to have this present for debugging purposes and trust users rather than to hide useful information that could help in resolving an issue.
mkindahl
force-pushed
the
fix-seclabel-error
branch
from
6572da1
to
cb34faa
Compare
mkindahl
force-pushed
the
fix-seclabel-error
branch
3 times, most recently
from
c3019d7
to
9b1c575
Compare
mkindahl
force-pushed
the
fix-seclabel-error
branch
3 times, most recently
from
1cc87ca
to
7dd3728
Compare
For all security labels from provider `timescaledb` an error was printed, which make `pg_restore` fail because the security labels are printed to the dump. Instead, we only print an error if the security label is not of the correct format. Fixes timescale#3760
mkindahl
force-pushed
the
fix-seclabel-error
branch
from
7dd3728
to
3fed2d0
Compare
For all security labels from provider
timescaledban error wasprinted, which make
pg_restorefail because the security labels areprinted to the dump.
Instead, we only print an error if the security label is not of the
correct format.
Fixes #3760