Re: Better support of exported snapshots with pg_dump

On Mon, Oct 27, 2014 at 7:37 PM, Petr Jelinek <petr(at)2ndquadrant(dot)com> wrote:
> I have two minor things:
> + printf(_(" --snapshot use given synchronous
> snapshot for the dump\n"));
Thanks for your input!

> I think this should say --snapshot=NAME or something like that to make it
> visible that you are supposed to provide the parameter.
Right. Let's do --snapshot=SNAPSHOT then.

> The other thing is, you changed logic of the parallel dump behavior a little
> - before your patch it works in a way that one connection exports snapshot
> and others do "SET TRANSACTION SNAPSHOT", after your patch, even the
> connection that exported the snapshot does the "SET TRANSACTION SNAPSHOT"
> which is unnecessary. I don't see it as too big deal but I don't see the
> need to change that behavior either.
Indeed, let's save some resources and not have the connection
exporting the snapshot use SET TRANSACTION SNAPSHOT if that's not
necessary.

> You could do something like this instead maybe?:
> if (AH->sync_snapshot_id)
> SET TRANSACTION SNAPSHOT
> else if (AH->numWorkers > 1 && AH->remoteVersion >= 90200 &&
> !dopt->no_synchronized_snapshots)
> AH->sync_snapshot_id = get_synchronized_snapshot(AH);
> And remove the else if for the if (dumpsnapshot) part.
Right as well. We still need to check for dumpsnapshot to set up
sync_snapshot_id for the first connection such as it can pass the
value to the other workers though.

Updated patch with those comments addressed is attached.
Regards,
--
Michael