Re: [REVIEW] In-core regression tests for replication, cascading, archiving, PITR, etc.

Поиск
Список
Период
Сортировка
От Craig Ringer
Тема Re: [REVIEW] In-core regression tests for replication, cascading, archiving, PITR, etc.
Дата
Msg-id CAMsr+YFA7aj3FygkVQDQNeEZtqOZuYvvCcmNzn706WVCBB3p=w@mail.gmail.com
обсуждение исходный текст
Ответ на Re: [REVIEW] In-core regression tests for replication, cascading, archiving, PITR, etc.  (Michael Paquier <michael.paquier@gmail.com>)
Ответы Re: [REVIEW] In-core regression tests for replication, cascading, archiving, PITR, etc.  (Michael Paquier <michael.paquier@gmail.com>)
Список pgsql-hackers
On 26 February 2016 at 10:58, Michael Paquier <michael.paquier@gmail.com> wrote:
 

Here is a rebased set after the conflicts created by e640093, with the
following changes:

Thanks for rebasing on top of that. Not totally fair when your patch came first, but I guess it was simpler to merge the other one first.
 
- In 0002, added perldoc for new promote routine
- In 0003, added perldoc documentation for the new options introduced
in init and init_from_backup, and fixed some log entries not using the
node name to identify the node involved when enabling archive,
streaming or recovery.

Very much appreciated.
 
- Craig has pinged me regarding tap_tests being incorrectly updated in
config_default.pl in 0003.
I just re-ran the tests on OSX and Windows (MSVC 2010 with Win7) to be
sure that nothing broke, and nothing has been reported as broken.



I've looked over the tests. I see that you've updated the docs for the Windows tests to reflect the changes, which is good, thanks.

I like the patch and would love to see it committed soon. 

I do have one major disagreement, which is that you turn autovacuum off if streaming is enabled. This is IMO completely wrong and must be removed. It's making the tests ignore a major and important part of real-world use.

If you did it to make it easier to test replay catchup etc, just use pg_xlog_location_diff instead of an equality test. Instead of:

my $caughtup_query = "SELECT '$current_lsn'::pg_lsn <= pg_last_xlog_replay_location()";

use

my $caughtup_query = "SELECT pg_xlog_location_diff('$current_lsn', pg_last_xlog_replay_location()) <= 0";

so it doesn't care if we replay past the expected LSN on the master due to autovacuum activity. That's what's done in the real world and what should be covered by the tests IMO.


The patch sets

    tap_tests => 1,

in config_default.pl. Was that on purpose? I'd have no problem with running the TAP tests by default if they worked by default, but the docs say that at least with ActiveState's Perl you have to jump through some hoops to get IPC::Run.

Typo in PostgresNode.pm: passiong should be 'passing' .


Otherwise looks _really_ good and I'd love to see this committed very soon.


I'd like a way to append parameters in a way that won't clobber settings made implicitly by the module through things like enable_streaming but I can add that in a followup patch. It doesn't need to complicate this one. I'm thinking of having the tests append an include_dir directive when they create a node, maintain a hash of all parameters and rewrite a postgresql.conf.taptests file in the include_dir when params are updated. Also exposing a 'reload' call.
 

--
 Craig Ringer                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

В списке pgsql-hackers по дате отправления:

Предыдущее
От: Kyotaro HORIGUCHI
Дата:
Сообщение: Re: IF (NOT) EXISTS in psql-completion
Следующее
От: Kyotaro HORIGUCHI
Дата:
Сообщение: Fixing wrong comment on PQmblen and PQdsplen.