Обсуждение: pg_dump -j against standbys

On 5/24/16 6:27 AM, Magnus Hagander wrote:
> pg_dump: Synchronized snapshots are not supported on standby servers.

Would it be that much harder to support this use case, perhaps by having 
the replica request the snapshot from the master on the client's behalf? 
It certainly doesn't surprise me that people would want to parallel dump 
from a replica...
-- 
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)   mobile: 512-569-9461

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.

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?)

Also, why didn't you keep using ExecuteSqlQueryForSingleRow()?
As coded, you're losing a sanity check that the query gives exactly
one row back.
        regards, tom lane

On Tue, May 24, 2016 at 7:02 AM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote:
> On 5/24/16 6:27 AM, Magnus Hagander wrote:
>>
>> pg_dump: Synchronized snapshots are not supported on standby servers.
>
>
> Would it be that much harder to support this use case, perhaps by having the
> replica request the snapshot from the master on the client's behalf? It
> certainly doesn't surprise me that people would want to parallel dump from a
> replica...

For HEAD, I totally agree. However, it seems to me that what Magnus is
looking for here is a backpatch that would allow users to get more
useful information related to the error that's happening: pg_dump
provides now an error that does not help at all in diagnosing what the
problem is. And that is confusing.
-- 
Michael

On 25/05/16 15:59, Magnus Hagander wrote:
> On Tue, May 24, 2016 at 5:39 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> 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?

You could try blaming Magnus.  Oh, wait..


.m

Magnus Hagander <magnus@hagander.net> writes:
>> Also, why didn't you keep using ExecuteSqlQueryForSingleRow()?

> 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?

No objection here, since it wouldn't be exposed outside pg_dump in any
case.
        regards, tom lane

Magnus Hagander <magnus@hagander.net> writes:
> Here's an updated patch based on this,and the other feedback.

Looks sane in a quick once-over, but I haven't tested it.
        regards, tom lane