Make PostgresNode::append_conf append a newline automatically.

tglsfdc · tglsfdc · commit 8a19c1a3736c · 2017-04-22T16:58:15.000-04:00
Although the documentation for append_conf said clearly that it didn't add a newline, many test authors seem to have forgotten that ... or maybe they just consulted the example at the top of the POD documentation, which clearly shows adding a config entry without bothering to add a trailing newline. The worst part of that is that it works, as long as you don't do it more than once, since the backend isn't picky about whether config files end with newlines. So there's not a strong forcing function reminding test authors not to do it like that. Upshot is that this is a terribly fragile way to go about things, and there's at least one existing test case that is demonstrably broken and not testing what it thinks it is. Let's just make append_conf append a newline, instead; that is clearly way safer than the old definition. I also cleaned up a few call sites that were unnecessarily ugly. (I left things alone in places where it's plausible that additional config lines would need to be added someday.) Back-patch the change in append_conf itself to 9.6 where it was added, as having a definitional inconsistency between branches would obviously be pretty hazardous for back-patching TAP tests. The other changes are just cosmetic and don't need to be back-patched. Discussion: https://postgr.es/m/19751.1492892376@sss.pgh.pa.us
diff --git a/src/test/modules/commit_ts/t/004_restart.pl b/src/test/modules/commit_ts/t/004_restart.pl
@@ -7,10 +7,7 @@
 
 my $node_master = get_new_node('master');
 $node_master->init(allows_streaming => 1);
-$node_master->append_conf(
-	'postgresql.conf', qq(
-track_commit_timestamp = on
-));
+$node_master->append_conf('postgresql.conf', 'track_commit_timestamp = on');
 $node_master->start;
 
 my ($ret, $stdout, $stderr);
@@ -75,10 +72,7 @@
 
 # Now disable commit timestamps
 
-$node_master->append_conf(
-	'postgresql.conf', qq(
-track_commit_timestamp = off
-));
+$node_master->append_conf('postgresql.conf', 'track_commit_timestamp = off');
 
 $node_master->stop('fast');
 $node_master->start;
@@ -110,10 +104,7 @@
 	'expected error from disabled tx when committs disabled');
 
 # Re-enable, restart and ensure we can still get the old timestamps
-$node_master->append_conf(
-	'postgresql.conf', qq(
-track_commit_timestamp = on
-));
+$node_master->append_conf('postgresql.conf', 'track_commit_timestamp = on');
 
 $node_master->stop('fast');
 $node_master->start;
diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm
@@ -455,7 +455,7 @@ A shortcut method to append to files like pg_hba.conf and postgresql.conf.
 Does no validation or sanity checking. Does not reload the configuration
 after writing.
 
-A newline is NOT automatically appended to the string.
+A newline is automatically appended to the string.
 
 =cut
 
@@ -465,7 +465,7 @@ sub append_conf
 
 	my $conffile = $self->data_dir . '/' . $filename;
 
-	TestLib::append_to_file($conffile, $str);
+	TestLib::append_to_file($conffile, $str . "\n");
 }
 
 =pod
diff --git a/src/test/recovery/t/001_stream_rep.pl b/src/test/recovery/t/001_stream_rep.pl
@@ -113,16 +113,16 @@ sub test_target_session_attrs
 # standbys. Since we're going to be testing things that affect the slot state,
 # also increase the standby feedback interval to ensure timely updates.
 my ($slotname_1, $slotname_2) = ('standby_1', 'standby_2');
-$node_master->append_conf('postgresql.conf', "max_replication_slots = 4\n");
+$node_master->append_conf('postgresql.conf', "max_replication_slots = 4");
 $node_master->restart;
 is($node_master->psql('postgres', qq[SELECT pg_create_physical_replication_slot('$slotname_1');]), 0, 'physical slot created on master');
-$node_standby_1->append_conf('recovery.conf', "primary_slot_name = $slotname_1\n");
-$node_standby_1->append_conf('postgresql.conf', "wal_receiver_status_interval = 1\n");
-$node_standby_1->append_conf('postgresql.conf', "max_replication_slots = 4\n");
+$node_standby_1->append_conf('recovery.conf', "primary_slot_name = $slotname_1");
+$node_standby_1->append_conf('postgresql.conf', "wal_receiver_status_interval = 1");
+$node_standby_1->append_conf('postgresql.conf', "max_replication_slots = 4");
 $node_standby_1->restart;
 is($node_standby_1->psql('postgres', qq[SELECT pg_create_physical_replication_slot('$slotname_2');]), 0, 'physical slot created on intermediate replica');
-$node_standby_2->append_conf('recovery.conf', "primary_slot_name = $slotname_2\n");
-$node_standby_2->append_conf('postgresql.conf', "wal_receiver_status_interval = 1\n");
+$node_standby_2->append_conf('recovery.conf', "primary_slot_name = $slotname_2");
+$node_standby_2->append_conf('postgresql.conf', "wal_receiver_status_interval = 1");
 $node_standby_2->restart;
 
 sub get_slot_xmins
diff --git a/src/test/recovery/t/002_archiving.pl b/src/test/recovery/t/002_archiving.pl
@@ -23,10 +23,8 @@
 my $node_standby = get_new_node('standby');
 $node_standby->init_from_backup($node_master, $backup_name,
 	has_restoring => 1);
-$node_standby->append_conf(
-	'postgresql.conf', qq(
-wal_retrieve_retry_interval = '100ms'
-));
+$node_standby->append_conf('postgresql.conf',
+	"wal_retrieve_retry_interval = '100ms'");
 $node_standby->start;
 
 # Create some content on master
diff --git a/src/test/recovery/t/003_recovery_targets.pl b/src/test/recovery/t/003_recovery_targets.pl
@@ -23,9 +23,7 @@ sub test_recovery_standby
 	foreach my $param_item (@$recovery_params)
 	{
 		$node_standby->append_conf(
-			'recovery.conf',
-			qq($param_item
-));
+			'recovery.conf', qq($param_item));
 	}
 
 	$node_standby->start;

Original file line number	Diff line number	Diff line change
`@@ -23,9 +23,7 @@ sub test_recovery_standby`
`23`	`23`	`foreach my $param_item (@$recovery_params)`
`24`	`24`	`{`
`25`	`25`	`$node_standby->append_conf(`
`26`		`- 'recovery.conf',`
`27`		`- qq($param_item`
`28`		`-));`
	`26`	`+ 'recovery.conf', qq($param_item));`
`29`	`27`	`}`
`30`	`28`
`31`	`29`	`$node_standby->start;`