PostgresNode::append_conf considered dangerous

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: pgsql-hackers(at)postgreSQL(dot)org
Subject: PostgresNode::append_conf considered dangerous
Date: 2017-04-22 20:19:36
Message-ID: 19751.1492892376@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2017-04-22 20:22:59 Re: A note about debugging TAP failures
Previous Message Noah Misch 2017-04-22 20:14:08 Re: pg_dump emits ALTER TABLE ONLY partitioned_table