Bug fix for cache lookup failure for statistic_ext type

Started by Mark Dilgerover 4 years ago4 messages
#1Mark Dilger
Mark Dilger
mark.dilger@enterprisedb.com
1 attachment(s)

Hackers,

You can easily get a cache lookup failure by changing the regression tests as included in this small patch. The failure looks thus:

+COMMENT ON STATISTICS ab1_a_b_stats IS 'new comment';
+CREATE ROLE temp_role;
+SET SESSION AUTHORIZATION temp_role;
+COMMENT ON STATISTICS ab1_a_b_stats IS 'changed comment';
+ERROR:  cache lookup failed for type 27447
+DROP STATISTICS ab1_a_b_stats;
+ERROR:  cache lookup failed for type 27447
+ALTER STATISTICS ab1_a_b_stats RENAME TO ab1_a_b_stats_new;
+ERROR:  must be owner of statistics object ab1_a_b_stats
+RESET SESSION AUTHORIZATION;
+DROP ROLE temp_role;

I believe this case simply has not had any test coverage, as I don't see any way the current code would ever work. It treats the Oid of the statistics object as a type, which it is not.

Attachments:

v1-0001-Fix-cache-lookup-error-in-ownership-check.patchapplication/octet-stream; name=v1-0001-Fix-cache-lookup-error-in-ownership-check.patch; x-unix-mode=0644
#2Tomas Vondra
Tomas Vondra
tomas.vondra@enterprisedb.com
In reply to: Mark Dilger (#1)
Re: Bug fix for cache lookup failure for statistic_ext type

On 8/5/21 12:03 AM, Mark Dilger wrote:

Hackers,

You can easily get a cache lookup failure by changing the regression tests as included in this small patch. The failure looks thus:

+COMMENT ON STATISTICS ab1_a_b_stats IS 'new comment';
+CREATE ROLE temp_role;
+SET SESSION AUTHORIZATION temp_role;
+COMMENT ON STATISTICS ab1_a_b_stats IS 'changed comment';
+ERROR:  cache lookup failed for type 27447
+DROP STATISTICS ab1_a_b_stats;
+ERROR:  cache lookup failed for type 27447
+ALTER STATISTICS ab1_a_b_stats RENAME TO ab1_a_b_stats_new;
+ERROR:  must be owner of statistics object ab1_a_b_stats
+RESET SESSION AUTHORIZATION;
+DROP ROLE temp_role;

I believe this case simply has not had any test coverage, as I don't
see any way the current code would ever work. It treats the Oid of the
statistics object as a type, which it is not.

Yeah, you're right - this is broken since 7b504eb282c. Thanks for the
fix, I'll get it committed.

regards

--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

#3Tomas Vondra
Tomas Vondra
tomas.vondra@enterprisedb.com
In reply to: Tomas Vondra (#2)
Re: Bug fix for cache lookup failure for statistic_ext type

On 8/5/21 12:32 AM, Tomas Vondra wrote:

On 8/5/21 12:03 AM, Mark Dilger wrote:

Hackers,

You can easily get a cache lookup failure by changing the regression
tests as included in this small patch.  The failure looks thus:

+COMMENT ON STATISTICS ab1_a_b_stats IS 'new comment';
+CREATE ROLE temp_role;
+SET SESSION AUTHORIZATION temp_role;
+COMMENT ON STATISTICS ab1_a_b_stats IS 'changed comment';
+ERROR:  cache lookup failed for type 27447
+DROP STATISTICS ab1_a_b_stats;
+ERROR:  cache lookup failed for type 27447
+ALTER STATISTICS ab1_a_b_stats RENAME TO ab1_a_b_stats_new;
+ERROR:  must be owner of statistics object ab1_a_b_stats
+RESET SESSION AUTHORIZATION;
+DROP ROLE temp_role;

I believe this case simply has not had any test coverage, as I don't
see any way the current code would ever work.  It treats the Oid of the
statistics object as a type, which it is not.

Yeah, you're right - this is broken since 7b504eb282c. Thanks for the
fix, I'll get it committed.

I've pushed a fix for this. And then a fix for the fix :-( because I
forgot about the rule that role names in regression tests should start
with regress_ prefix, so animals enforcing this failed.

regards

--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

#4Mark Dilger
Mark Dilger
mark.dilger@enterprisedb.com
In reply to: Tomas Vondra (#3)
Re: Bug fix for cache lookup failure for statistic_ext type

On Aug 31, 2021, at 10:50 AM, Tomas Vondra <tomas.vondra@enterprisedb.com> wrote:

I've pushed a fix for this. And then a fix for the fix :-( because I forgot about the rule that role names in regression tests should start with regress_ prefix, so animals enforcing this failed.

Thanks!


Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company