Horton

Thread

Commits

  1. Make PostgresNode::append_conf append a newline automatically.

  1. PostgresNode::append_conf considered dangerous

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-04-22T20:19:36Z

    Okay, so append_conf is perfectly straightforward about what it does:

	A newline is NOT automatically appended to the string.

However, I count at least eight places that are being lazy, like this:

  # Change a setting and restart
  $node->append_conf('postgresql.conf', 'hot_standby = on');
  $node->restart();

The worst part is that that works, as long as you don't do it more
than once in a script.  When you do it twice, though, you have a
syntactically invalid config file.  Thus for example this bit in
src/test/modules/commit_ts/t/003_standby_2.pl isn't doing anywhere
near what it thinks it's doing:

$master->append_conf('postgresql.conf', 'track_commit_timestamp = on');
$master->restart;
$master->append_conf('postgresql.conf', 'track_commit_timestamp = off');
$master->restart;

(I found that after deciding that PostgresNode::restart should complain
if pg_ctl failed.  The node is in fact not running after this, but the
existing test script fails to notice that.)

Now we could run around and make all these places do it the "right way",
like this other call:

$node_master->append_conf(
	'postgresql.conf', qq(
track_commit_timestamp = on
));

But (1) that's pretty darn ugly code, and (2) this fix does not prevent
future mistakes of the same ilk, which are obviously easy to make.

I think instead we should change append_conf to append a newline, and
simplify the callers that could thereby be simplified.  I don't see
any callers that would be broken by that; and we do not have any config
files in which extra blank lines are problematic.

			regards, tom lane

  2. Re: PostgresNode::append_conf considered dangerous

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2017-04-23T00:56:28Z

    
On 04/22/2017 04:19 PM, Tom Lane wrote:
>
> I think instead we should change append_conf to append a newline, and
> simplify the callers that could thereby be simplified.  I don't see
> any callers that would be broken by that; and we do not have any config
> files in which extra blank lines are problematic.
>
> 			


+1. Not sure how we survived so long with it the way it is.

cheers

andrew

-- 
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services