On Tue, May 24, 2016 at 5:39 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:
> Magnus Hagander <mag...@hagander.net> writes:
> > I think the cleanest way to do it is to just track if it's a standby in
> > 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?