Thanks for the review. On Tue, Nov 24, 2015 at 6:15 AM, Alvaro Herrera <alvhe...@2ndquadrant.com> wrote:
> I just noticed that RecoveryTest.pm is lacking "use strict; use > warnings;". With those added, there's a number of problems reported: > Most of them are easily fixable by adding the correct "my" lines; but at > least @array and $current_dir require more code to be written. > Oops. > TBH all that business with arrays that are kept in sync looks too > contrived to me. Could we have a Perl object representing each node > instead? > Not really to be honest. > That would require a "PostgresNode" package (or similar). The > RecoveryTest.pm would have a single %nodes hash. Also, you don't need > @active_nodes, just a flag in PostgresNode, and have the stop routine do > nothing if node is not marked active. Also: if you pass the "root node" > when creating a node that will become a standby, you don't need to pass > it when calling, say, enable_streaming; the root node becomes an > instance variable. (Hmm, actually, if we do that, I wonder what if in > the future we want to test node promotion and a standby is repointed to > a new master. Maybe we don't want to have this knowledge in the Perl > code at all.) > I think I'll get the idea. In short all the parametrization will just happen at object level, as well as basic actions on the nodes like start, stop, restart etc. > In get_free_port, isn't it easier to use pg_isready rather than psql? > Will switch. > I've been messing with 003 because I think it's a bit too repetitive. > Will finish it after you post a fixed version of RecoveryTest.pm. > Sure, thanks. I'll rework this patch and will update a new version soon. Thanks again for the review. -- Michael
