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 > 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? -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
