Hi Surya, Chao >> PFA v4: >> >> * 0001 unchanged >> * 0002 added the tests that Kiran suggested
Thanks for the review, Surya. I tested v4 on the same base (e8a3ee5b197) and results are inline with your findings > 1. In subscription.sql, I don't think the session-authorization is needed. > regress_subscription_user is created as LOGIN SUPERUSER at the top of the > file and is the active role > from line 190 onwards, so the session is already superuser when the new block > runs. > I removed the RESET/SET pair locally and the subscription test still passes, > so the superuser bypass over > pg_replication_origin_advance's restricted ACL is enough. > Dropping them also keeps the new test running as the subscription owner, > which is what every other SKIP test in the file does. Agree on this! > 2. The first comment in the new block looks misplaced: > -- fail - LSN must not be behind the replication origin > RESET SESSION AUTHORIZATION; > > RESET SESSION AUTHORIZATION neither fails nor has anything to do with the > origin, and the comment duplicates the one further down above the statement > it actually describes. I think it can just be removed. Agree on this! > 3. Still in that block, pg_replication_origin_advance() returns > void, so its output is stable across runs and there is nothing for \gset to > suppress. A plain terminated statement would read more naturally and > wouldn't leave a psql variable behind. Agree on 3. The \gset here is only hiding an empty one-row result, and a plain statement says what it means. Worth noting it adds five lines to the expected file, which seems a fair trade. > 4. Very minor: the new WATCH_INTERVAL test uses > > '\set WATCH_INTERVAL 1000000' . "\n" . '\echo :WATCH_INTERVAL' > where the surrounding tests in 001_basic.pl use a literal multiline > single-quoted string. Point 4 is my mistake, not Chao's. That concatenation came from the snippet I sent upthread and he may have used it as-is. Sorry for the noise. Thanks, Kiran Kaki.
