On Tue, May 24, 2016 at 5:39 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Magnus Hagander <magnus@hagander.net> writes: > I think the cleanest way to do it is to just track if it's a standby in the > AH struct as written.
> Comments?
This patch will cause pg_dump to fail entirely against pre-9.0 servers. You need to execute the test conditionally.
Ugh. can I blame coding while too jetlagged?
Also, in view of that, this test
- if (fout->remoteVersion >= 90000) + if (fout->remoteVersion >= 90000 && fout->isStandby)
could be reduced to just "if (fout->isStandby)". And the comment in front of it could stand some attention (possibly just replace it with the one that's currently within the inner if() ... actually, that comment should move to where you moved the test to, no?)
True. Will fix.
Also, why didn't you keep using ExecuteSqlQueryForSingleRow()? As coded, you're losing a sanity check that the query gives exactly one row back.
The reason I did that is that ExecuteSqlQueryForSingleRow() is a static method in pg_dump.c. I was planning to go back and review that, and consider moving it, but I forgot it :S
I think the clean thing is probably to use that one, and also move it over to not be a static method in pg_dump.c, but instead sit next to ExecuteSqlQuery in pg_backup_db.c. Do you agree that's reasonable, and something that's OK to backpatch?