The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:       not tested
Spec compliant:           not tested
Documentation:            tested, failed

Hi!

I read through the discussion. I agree with the idea here. I think if DISCARD 
ALL is allowed during hot standby mode, and it includes UNLISTEN *, only 
UNLISTEN * should also be allowed. It seems this patch fixes this, but I could 
not test it (I do not know how to force this state). I would go further though, 
and I would also allow UNLISTEN a. Because also DISCARD allows discarding only 
part of resources.

So patch looks good to me, but documentation changes are missing from it. 
UNLISTEN should be removed from the list of commands not allowed and moved to 
the list of those which are allowed [1]. Moreover, 
src/test/regress/sql/hs_standby_allowed.sql and 
src/test/regress/sql/hs_standby_disallowed.sql tests should be updated based on 
these changes in the patch. What is surprising to me is that make check-world 
does not fail with this change, but there is an explicit check for UNLISTEN *. 
So does this mean those tests are not run or does it mean that this patch does 
not fix the problem?

[1] https://www.postgresql.org/docs/current/hot-standby.html#HOT-STANDBY-USERS


Mitar

The new status of this patch is: Waiting on Author

Reply via email to