COMMENT ON current_database() IS NULL <-- current_database() throws error

[chadfurman]

The current_database command in this context breaks. I simply remove this line from the generated output.

[zstojanovic]

I can confirm this issue. It's simple to reproduce, just change comment on database between old and new SQL dump, and apgdiff will generate upgrade statement similar to this:

COMMENT ON DATABASE current_database() IS 'Some comment';

But when this is executed, postgres complains:

ERROR:  syntax error at or near "("
LINE 1: COMMENT ON DATABASE current_database() IS 'Some comment';

It seems that calling current_database() is not allowed here.

I tried to look into possible solutions for this in apgdiff code. If I understand correctly, the problem is that apgdiff doesn't know the actual name of the database so that's why the original author tried using a call to current_database() in generated upgrade statement.

Possible solution would be to change apgdiff code to generate upgrade statement using anonymous code block like this:

do $$
BEGIN
EXECUTE 'COMMENT ON DATABASE "' || current_database() || '" IS ''Some comment'';';
END
$$;

If it's OK with everybody, I can try to take a stab at creating a pull request for this.

[chadfurman]

That's an interesting solution -- I'm hesitant to use EXECUTE though, as depending on where it gets used it might lead to unexpected SQL injections

I can't think of anything right now, though, that stands out as an immediate bad idea. However, there are a lot of advanced features of postgres and I didn't write this library :)

[zstojanovic]

I'm not sure EXECUTE has any more potential for SQL injection than other DDL that apgdiff uses. Also, apgdiff correctly escapes single quotes with two single quotes in generated upgrade scripts which should be enough protection against SQL injection.

Having said that, I too am hesitant about this solution.

For one, it does look a bit ugly and hacky to use an anonymous block just as a workaround for this one case. Another concern might be that this would only work for PostgreSQL version 9.0 and newer (not sure what versions apgdiff currently supports).

I the meantime, I found out that PostgreSQL might introduce CURRENT_DATABASE keyword which could be another approach to solve this.

Now I'm not sure what the appropriate solution would be, so I'll wait for some kind of consensus on the issue.

[chadfurman]

I'm not sure what versions of postgres are supported at this point in time. I'm only comfortable supporting the latest version of Postgres which runs on AWS RDS, myself.

I'm not too worried about the ugly hackish-ness if we can get consensus that it's relatively safe

[chadfurman]

also current_database keyword sounds nice though I'm not gonna hold my breath for it to land :)

[zstojanovic]

Yeah, I wouldn't hold my breath for the new keyword either :)

Regarding safety of the use of EXECUTE, I personally don't see any possibility for an SQL injection since the strings are already properly escaped, and I also don't see that It would be any different from what apgdiff is already using. Nevertheless it would be best if somebody else also weighs in on this.

[jflambert]

I'm pretty sure the answer is "no" but was this fixed with the recently released 2.6-1? As a workaround, I've been grepping out lines that begin with "COMMENT ON".