Re: Patch for Prevent pg_dump/pg_restore from being affected by statement_timeout

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hello,

O.k. this appeared easy enough for even me to do it. So I did. It seems
to work but I wasn't 100% positive on "where" I put the code changes.
Please take a look.

Sincerely,

Joshua D. Drake

- --
The PostgreSQL Company since 1997: http://www.commandprompt.com/
PostgreSQL Community Conference: http://www.postgresqlconference.org/
Donate to the PostgreSQL Project: http://www.postgresql.org/about/donate
PostgreSQL political pundit | Mocker of Dolphins

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)

iD8DBQFH1sVyATb/zqfZUUQRAkO/AJ4jncdM3NbbwXCVngitkadxxTAGawCeMBeZ
Lnr2zCdV1WLijQl+dE5yUgU=
=Qu8m
-----END PGP SIGNATURE-----

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On Tue, 11 Mar 2008 10:46:23 -0700
"Joshua D. Drake" <jd(at)commandprompt(dot)com> wrote:

And the -c version :) (thanks bruce)

Joshua D. Drake

- --
The PostgreSQL Company since 1997: http://www.commandprompt.com/
PostgreSQL Community Conference: http://www.postgresqlconference.org/
Donate to the PostgreSQL Project: http://www.postgresql.org/about/donate
PostgreSQL political pundit | Mocker of Dolphins

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)

iD8DBQFH1spfATb/zqfZUUQRAv2EAJ92/8EBxBbqLDlOX5wUYdN3ElG5OQCghZ2Z
tUIrN/MYVgP6rc4QXONDrFg=
=2oJ5
-----END PGP SIGNATURE-----

On Tue, 11 Mar 2008 11:07:27 -0700
"Joshua D. Drake" <jd(at)commandprompt(dot)com> wrote:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On Tue, 11 Mar 2008 10:46:23 -0700
> "Joshua D. Drake" <jd(at)commandprompt(dot)com> wrote:
>
> And the -c version :) (thanks bruce)
>
> Joshua D. Drake
>

What is the feedback on this patch? Is there anything I need to do to
get it into the next commit fest?

Joshua D. Drake

--
The PostgreSQL Company since 1997: http://www.commandprompt.com/
PostgreSQL Community Conference: http://www.postgresqlconference.org/
United States PostgreSQL Association: http://www.postgresql.us/
Donate to the PostgreSQL Project: http://www.postgresql.org/about/donate

Joshua D. Drake wrote:

> What is the feedback on this patch? Is there anything I need to do to
> get it into the next commit fest?

Please add it to the commitfest wiki page.

http://wiki.postgresql.org/wiki/CommitFest:May

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Wed, 16 Apr 2008 13:36:50 -0400
Alvaro Herrera <alvherre(at)commandprompt(dot)com> wrote:

> Joshua D. Drake wrote:
>
> > What is the feedback on this patch? Is there anything I need to do
> > to get it into the next commit fest?
>
> Please add it to the commitfest wiki page.
>
> http://wiki.postgresql.org/wiki/CommitFest:May
>

Done: http://wiki.postgresql.org/wiki/CommitFest:May#Pending_patches

Joshua D. Drake

--
The PostgreSQL Company since 1997: http://www.commandprompt.com/
PostgreSQL Community Conference: http://www.postgresqlconference.org/
United States PostgreSQL Association: http://www.postgresql.us/
Donate to the PostgreSQL Project: http://www.postgresql.org/about/donate

Joshua D. Drake wrote:
> What is the feedback on this patch? Is there anything I need to do to
> get it into the next commit fest?

Yes, go add it to the wiki page ;-):
http://wiki.postgresql.org/wiki/CommitFest:May

I agree that we should do that, but the thread on -hackers ("Autovacuum
vs statement_timeout") wasn't totally conclusive. Greg Sabine Mullane
and Peter Eisentraut argued that we shouldn't, but neither provided a
plausible use case where a statement_timeout on restoring a dump would
be useful. I'm thinking we should apply the patch unless someone comes
up with one.

To quote Tom:
> I think we need to be careful to distinguish three situations:
>
> * statement_timeout during pg_dump
> * statement_timeout during pg_restore
> * statement_timeout during psql reading a pg_dump script file

This patch addresses the third situation, but leaves open the 1st and
the 2nd. IMO, we should set statement_timeout = 0 in them as well,
unless someone comes up with plausible use case for using a non-zero
statement_timeout.

Ps. If you want to save the committer a couple of minutes of valuable
time, you can fix the indentation to use tabs instead of spaces, and
remove the spurious whitespace change on the empty line.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

On Wed, 16 Apr 2008 21:04:17 +0300
Heikki Linnakangas <heikki(at)enterprisedb(dot)com> wrote:

> To quote Tom:
> > I think we need to be careful to distinguish three situations:
> >
> > * statement_timeout during pg_dump
> > * statement_timeout during pg_restore
> > * statement_timeout during psql reading a pg_dump script file
>
> This patch addresses the third situation, but leaves open the 1st and
> the 2nd. IMO, we should set statement_timeout = 0 in them as well,
> unless someone comes up with plausible use case for using a non-zero
> statement_timeout.

My patch addresses all three, unless I am misunderstanding your
meaning. The patch does the following:

After connection with pg_dump it executes set statement_timeout = 0;
This fixed the pg_dump timeout issue.

It also writes set statement_timeout = 0 into the archive file, which
fixed pg_restore and psql.

>
> Ps. If you want to save the committer a couple of minutes of valuable
> time, you can fix the indentation to use tabs instead of spaces, and
> remove the spurious whitespace change on the empty line.
>

I can do that. Thanks for the feedback.

Joshua D. Drake

--
The PostgreSQL Company since 1997: http://www.commandprompt.com/
PostgreSQL Community Conference: http://www.postgresqlconference.org/
United States PostgreSQL Association: http://www.postgresql.us/
Donate to the PostgreSQL Project: http://www.postgresql.org/about/donate

Joshua D. Drake wrote:
> On Wed, 16 Apr 2008 21:04:17 +0300
> Heikki Linnakangas <heikki(at)enterprisedb(dot)com> wrote:
>> To quote Tom:
>>> I think we need to be careful to distinguish three situations:
>>>
>>> * statement_timeout during pg_dump
>>> * statement_timeout during pg_restore
>>> * statement_timeout during psql reading a pg_dump script file
>> This patch addresses the third situation, but leaves open the 1st and
>> the 2nd. IMO, we should set statement_timeout = 0 in them as well,
>> unless someone comes up with plausible use case for using a non-zero
>> statement_timeout.
>
> My patch addresses all three, unless I am misunderstanding your
> meaning. The patch does the following:
>
> After connection with pg_dump it executes set statement_timeout = 0;
> This fixed the pg_dump timeout issue.
>
> It also writes set statement_timeout = 0 into the archive file, which
> fixed pg_restore and psql.

Oh, ok, I misread the patch. Sorry for the noise.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

On Wed, 16 Apr 2008 21:20:17 +0300
Heikki Linnakangas <heikki(at)enterprisedb(dot)com> wrote:

> > My patch addresses all three, unless I am misunderstanding your
> > meaning. The patch does the following:
> >
> > After connection with pg_dump it executes set statement_timeout = 0;
> > This fixed the pg_dump timeout issue.
> >
> > It also writes set statement_timeout = 0 into the archive file,
> > which fixed pg_restore and psql.
>
> Oh, ok, I misread the patch. Sorry for the noise.
>

:)

Joshua D. Drake

--
The PostgreSQL Company since 1997: http://www.commandprompt.com/
PostgreSQL Community Conference: http://www.postgresqlconference.org/
United States PostgreSQL Association: http://www.postgresql.us/
Donate to the PostgreSQL Project: http://www.postgresql.org/about/donate

-----BEGIN PGP SIGNED MESSAGE-----
Hash: RIPEMD160

> I agree that we should do that, but the thread on -hackers ("Autovacuum
> vs statement_timeout") wasn't totally conclusive. Greg Sabine Mullane
> and Peter Eisentraut argued that we shouldn't, but neither provided a
> plausible use case where a statement_timeout on restoring a dump would
> be useful. I'm thinking we should apply the patch unless someone comes
> up with one.

I don't think it's fair to simply discard the use cases provided as
"implausible" and demand one more to your liking. I strongly dislike
having a giant dump file written that has non-vital configuration
variables embedded in the top of it, precluding any user choice
whatsoever[1]. As before, where are the reports of all the people
having their manual restorations interrupted by a statement_timeout?

[1] Short of editing a potentially GB-size file, or using
some sed/shell shenanigans to strip out the suspect setting.

- --
Greg Sabino Mullane greg(at)turnstep(dot)com
End Point Corporation
PGP Key: 0x14964AC8 200804161814
http://biglumber.com/x/web?pk=2529DF6AB8F79407E94445B4BC9B906714964AC8
-----BEGIN PGP SIGNATURE-----

iEYEAREDAAYFAkgGetEACgkQvJuQZxSWSsg+4ACghvlBkIth1aBiGpFPFPj+HWgf
iyEAnj+WK9MQL+ZQqKoTcLOe/lvoNkkV
=Nlyg
-----END PGP SIGNATURE-----

On Wed, 16 Apr 2008 22:17:30 -0000
"Greg Sabino Mullane" <greg(at)turnstep(dot)com> wrote:

> I don't think it's fair to simply discard the use cases provided as
> "implausible" and demand one more to your liking. I strongly dislike
> having a giant dump file written that has non-vital configuration
> variables embedded in the top of it, precluding any user choice
> whatsoever[1]. As before, where are the reports of all the people
> having their manual restorations interrupted by a statement_timeout?

Calling me, wondering why in the world it is happening.

Joshua D. Drake

--
The PostgreSQL Company since 1997: http://www.commandprompt.com/
PostgreSQL Community Conference: http://www.postgresqlconference.org/
United States PostgreSQL Association: http://www.postgresql.us/
Donate to the PostgreSQL Project: http://www.postgresql.org/about/donate

On Wed, 16 Apr 2008 15:22:31 -0700
"Joshua D. Drake" <jd(at)commandprompt(dot)com> wrote:

> On Wed, 16 Apr 2008 22:17:30 -0000
> "Greg Sabino Mullane" <greg(at)turnstep(dot)com> wrote:
>
> > I don't think it's fair to simply discard the use cases provided as
> > "implausible" and demand one more to your liking. I strongly dislike
> > having a giant dump file written that has non-vital configuration
> > variables embedded in the top of it, precluding any user choice
> > whatsoever[1]. As before, where are the reports of all the people
> > having their manual restorations interrupted by a statement_timeout?
>
> Calling me, wondering why in the world it is happening.

Sorry couldn't help myself. Anyway, in an attempt to be productive, I
will say that your "where are all the reports" is about as valid as,
"Where are all the users besides yourself arguing about this having to
edit a backup file?"

This is a real problem and unless we can find more people to
substantiate your claim, I think the consensus is to ensure that people
don't get bit by statement timeout when attempting to do a restore.

I vote in favor of the one less foot gun approach.

Sincerely,

Joshua D. Drake

--
The PostgreSQL Company since 1997: http://www.commandprompt.com/
PostgreSQL Community Conference: http://www.postgresqlconference.org/
United States PostgreSQL Association: http://www.postgresql.us/
Donate to the PostgreSQL Project: http://www.postgresql.org/about/donate

On Wed, Apr 16, 2008 at 4:32 PM, Joshua D. Drake <jd(at)commandprompt(dot)com> wrote:
> On Wed, 16 Apr 2008 15:22:31 -0700
>
> "Joshua D. Drake" <jd(at)commandprompt(dot)com> wrote:
>
>
> > On Wed, 16 Apr 2008 22:17:30 -0000
> > "Greg Sabino Mullane" <greg(at)turnstep(dot)com> wrote:
> >
> > > I don't think it's fair to simply discard the use cases provided as
> > > "implausible" and demand one more to your liking. I strongly dislike
> > > having a giant dump file written that has non-vital configuration
> > > variables embedded in the top of it, precluding any user choice
> > > whatsoever[1]. As before, where are the reports of all the people
> > > having their manual restorations interrupted by a statement_timeout?
> >
> > Calling me, wondering why in the world it is happening.
>
> Sorry couldn't help myself. Anyway, in an attempt to be productive, I
> will say that your "where are all the reports" is about as valid as,
> "Where are all the users besides yourself arguing about this having to
> edit a backup file?"
>
> This is a real problem and unless we can find more people to
> substantiate your claim, I think the consensus is to ensure that people
> don't get bit by statement timeout when attempting to do a restore.
>
> I vote in favor of the one less foot gun approach.

Sorry if i missed the obvious reason not to do this... but if its a
command line option the user can choose. Why not something like this
(i did it for pg_dump only...)

diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index ed1b33d..bf9365a 100644
*** a/src/bin/pg_dump/pg_dump.c
--- /bsrc/bin/pg_dump/pg_dump.c
*************** main(int argc, char **argv)
*** 225,230 ****
--- 225,231 ----
int outputNoOwner = 0;
static int use_setsessauth = 0;
static int disable_triggers = 0;
+ static int use_statement_timeout = 0;
char *outputSuperuser = NULL;

RestoreOptions *ropt;
*************** main(int argc, char **argv)
*** 267,272 ****
--- 268,274 ----
{"disable-dollar-quoting", no_argument,
&disable_dollar_quoting, 1},
{"disable-triggers", no_argument, &disable_triggers, 1},
{"use-set-session-authorization", no_argument,
&use_setsessauth, 1},
+ {"use-statement-timeout", no_argument,
&use_statement_timeout, 1},

{NULL, 0, NULL, 0}
};
*************** main(int argc, char **argv)
*** 419,424 ****
--- 421,428 ----
disable_triggers = 1;
else if (strcmp(optarg,
"use-set-session-authorization") == 0)
use_setsessauth = 1;
+ else if (strcmp(optarg,
"use-statement-timeout") == 0)
+ use_statement_timeout = 1;
else
{
fprintf(stderr,
*************** main(int argc, char **argv)
*** 571,576 ****
--- 575,583 ----
*/
do_sql_command(g_conn, "BEGIN");

+ if (!use_statement_timeout)
+ do_sql_command(g_conn, "SET statement_timeout = 0;");
+
do_sql_command(g_conn, "SET TRANSACTION ISOLATION LEVEL SERIALIZABLE");

/* Select the appropriate subquery to convert user IDs to names */
*************** help(const char *progname)
*** 771,776 ****
--- 778,784 ----
printf(_(" --use-set-session-authorization\n"
" use SESSION
AUTHORIZATION commands instead of\n"
" ALTER OWNER commands to set
ownership\n"));
+ printf(_(" --use-statement-timeout respect statement_timeout\n"));

printf(_("\nConnection options:\n"));
printf(_(" -h, --host=HOSTNAME database server host or
socket directory\n"));

Alex Hunsaker wrote:
>
>
> Sorry if i missed the obvious reason not to do this... but if its a
> command line option the user can choose. Why not something like this
> (i did it for pg_dump only...)
>
>
>
Actually, it's probably more important to be selectable at restore time
than at dump time, so if you're doing just one ...

This whole thing set me wondering whether or not we should provide a
more general command-line facility to psql and pg_restore, and maybe
others, to do some session setup before running their commands.

Of course, there's no reason we couldn't have both.

cheers

andrew

On Wed, 16 Apr 2008 18:50:28 -0400
Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:

>
>
> Alex Hunsaker wrote:
> >
> >
> > Sorry if i missed the obvious reason not to do this... but if its a
> > command line option the user can choose. Why not something like
> > this (i did it for pg_dump only...)
> >
> >
> >
> Actually, it's probably more important to be selectable at restore
> time than at dump time, so if you're doing just one ...
>
> This whole thing set me wondering whether or not we should provide a
> more general command-line facility to psql and pg_restore, and maybe
> others, to do some session setup before running their commands.
>
> Of course, there's no reason we couldn't have both.

That is an interesting idea. Something like:

pg_restore -E "SET STATEMENT_TIMEOUT=0; SET MAINTENANCE_WORK_MEM=1G" ?

Sincerely,

Joshua D. Drake

>
> cheers
>
> andrew
>

--
The PostgreSQL Company since 1997: http://www.commandprompt.com/
PostgreSQL Community Conference: http://www.postgresqlconference.org/
United States PostgreSQL Association: http://www.postgresql.us/
Donate to the PostgreSQL Project: http://www.postgresql.org/about/donate

Joshua D. Drake escribió:
> On Wed, 16 Apr 2008 18:50:28 -0400
> Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:

> > Actually, it's probably more important to be selectable at restore
> > time than at dump time, so if you're doing just one ...

I think the patch posted by Joshua at the start of this thread does
that.

> > This whole thing set me wondering whether or not we should provide a
> > more general command-line facility to psql and pg_restore, and maybe
> > others, to do some session setup before running their commands.
>
> That is an interesting idea. Something like:
>
> pg_restore -E "SET STATEMENT_TIMEOUT=0; SET MAINTENANCE_WORK_MEM=1G" ?

We already have it -- it's called PGOPTIONS.

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Wed, Apr 16, 2008 at 4:54 PM, Alvaro Herrera
<alvherre(at)commandprompt(dot)com> wrote:
> Joshua D. Drake escribió:
>
> > That is an interesting idea. Something like:
> >
> > pg_restore -E "SET STATEMENT_TIMEOUT=0; SET MAINTENANCE_WORK_MEM=1G" ?
>
> We already have it -- it's called PGOPTIONS.
>

Ok but is not the purpose of the patch to turn off statement_timeout
by *default* in pg_restore/pg_dump?

Here is an updated patch for I posted above (with the command line
option --use-statement-timeout) for pg_dump and pg_restore.

(sorry If I hijacked your patch Josh :) )

Greg Sabino Mullane wrote:
>> I agree that we should do that, but the thread on -hackers ("Autovacuum
>> vs statement_timeout") wasn't totally conclusive. Greg Sabine Mullane
>> and Peter Eisentraut argued that we shouldn't, but neither provided a
>> plausible use case where a statement_timeout on restoring a dump would
>> be useful. I'm thinking we should apply the patch unless someone comes
>> up with one.
>
> I don't think it's fair to simply discard the use cases provided as
> "implausible" and demand one more to your liking.

Sorry if I missed it in the original thread, but what is the use case
you have in mind?

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

Heikki Linnakangas wrote:
> Sorry if I missed it in the original thread, but what is the use case
> you have in mind?

I think the bottom line is just that having statement_timeout a global setting
is stupid for a variety of reasons (dump, restore, vacuum, locks, incidental
delays) that we should discourage it (or prevent it, as proposed elsewhere)
rather than working around it in countless individual places.

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> Heikki Linnakangas wrote:
>> Sorry if I missed it in the original thread, but what is the use case
>> you have in mind?

> I think the bottom line is just that having statement_timeout a global setting
> is stupid for a variety of reasons (dump, restore, vacuum, locks, incidental
> delays) that we should discourage it (or prevent it, as proposed elsewhere)
> rather than working around it in countless individual places.

I'm not convinced that there's no use-case for global statement_timeout,
and even less convinced that there won't be anyone setting one anyway.
Unless we are prepared to somehow *prevent* such a setting from being
put in place, the proposed patch seems reasonable to me.

Unless you have a use-case in which it's actually desirable for the dump
or restore to fail. I'm having a tough time imagining one though.

regards, tom lane

-----BEGIN PGP SIGNED MESSAGE-----
Hash: RIPEMD160

> Sorry if I missed it in the original thread, but what is the
> use case you have in mind?

A use case would be dumping a large table and wanting to
load it into the database, but wanting to stop the job if it
is still running an hour from now, when a maintenance window
is scheduled to start.

However, I think my objection is more philosophical, as we've
changed from having pg_dump make a SQL representation of part
or all of your database, into also having it force what it
thinks should be the right environment as well. Yes, timeout
can be a foot gun, but it's a foot gun that off by default,
and must be explicitly turned on. The fact that a setting that
kills long-running queries ends up killing long-running queries
via psql -f seems worth a documentation warning, not a change
in the textual representation of a database. I checked the
archives and have yet to find a single instance in -bugs of
anyone complaining about this. The closest I found was someone
having problems with psql and -c, but they were specifically
aware of the timeout and were trying (unseccessfully) to
disable it first. For the record, I have no problem with
disabling the timeout in both pg_dump and pg_restore.

- --
Greg Sabino Mullane greg(at)turnstep(dot)com
PGP Key: 0x14964AC8 200804171250
http://biglumber.com/x/web?pk=2529DF6AB8F79407E94445B4BC9B906714964AC8
-----BEGIN PGP SIGNATURE-----

iEYEAREDAAYFAkgHf/sACgkQvJuQZxSWSsiXOwCggD1P/SgPwOO3gJdlXKP5bU3l
dWgAnRK5FNixLy8ajgkfI3Y/UpDyoeZB
=qaA5
-----END PGP SIGNATURE-----

Greg Sabino Mullane wrote:
> A use case would be dumping a large table and wanting to
> load it into the database, but wanting to stop the job if it
> is still running an hour from now, when a maintenance window
> is scheduled to start.

statement_timeout is pretty useless for that purpose, because it limits
the time on a per-statement basis. It would cancel the COPY of any
tables larger than X, but if you have multiple tables (a partitioned
table, perhaps) just below the threshold, they would all be dumped even
though the cumulative time is well beyond statement_timeout.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

On Thu, Apr 17, 2008 at 03:18:54PM +0200, Peter Eisentraut wrote:
> I think the bottom line is just that having statement_timeout a global setting
> is stupid for a variety of reasons (dump, restore, vacuum, locks, incidental
> delays) that we should discourage it (or prevent it, as proposed elsewhere)
> rather than working around it in countless individual places.

maintainence_statement_timeout? :)

Have a nice day,
--
Martijn van Oosterhout <kleptog(at)svana(dot)org> http://svana.org/kleptog/
> Please line up in a tree and maintain the heap invariant while
> boarding. Thank you for flying nlogn airlines.

Joshua D. Drake wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On Tue, 11 Mar 2008 10:46:23 -0700
> "Joshua D. Drake" <jd(at)commandprompt(dot)com> wrote:
>
> And the -c version :) (thanks bruce)
>
>
>

Committed with slight editorializing. Statement timeout was only
introduced in 7.3, whereas pg_dump can dump from much older versions of
Postgres.

Also, the indentation needed fixing.

wiki updated.

cheers

andrew

Andrew Dunstan wrote:

> Committed with slight editorializing. Statement timeout was only
> introduced in 7.3, whereas pg_dump can dump from much older versions of
> Postgres.
>
You forget a ; in this committ [1].

[1] http://archives.postgresql.org/pgsql-committers/2008-05/msg00028.php

--
Euler Taveira de Oliveira
http://www.timbira.com/

Euler Taveira de Oliveira wrote:
> Andrew Dunstan wrote:
>
>> Committed with slight editorializing. Statement timeout was only
>> introduced in 7.3, whereas pg_dump can dump from much older versions
>> of Postgres.
>>
> You forget a ; in this committ [1].
>
> [1] http://archives.postgresql.org/pgsql-committers/2008-05/msg00028.php
>

I need to stop doing things late at night.

fixed.

cheers

andrew.

Alex Hunsaker wrote:
> On Wed, Apr 16, 2008 at 4:54 PM, Alvaro Herrera
> <alvherre(at)commandprompt(dot)com> wrote:
> > Joshua D. Drake escribi?:
> >
> > > That is an interesting idea. Something like:
> > >
> > > pg_restore -E "SET STATEMENT_TIMEOUT=0; SET MAINTENANCE_WORK_MEM=1G" ?
> >
> > We already have it -- it's called PGOPTIONS.
> >
>
> Ok but is not the purpose of the patch to turn off statement_timeout
> by *default* in pg_restore/pg_dump?
>
> Here is an updated patch for I posted above (with the command line
> option --use-statement-timeout) for pg_dump and pg_restore.

I would like to get do this without adding a new --use-statement-timeout
flag. Is anyone going to want to honor statement_timeout during
pg_dump/pg_restore? I thought we were just going to disable it.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

On Mon, Jun 23, 2008 at 06:51:28PM -0400, Bruce Momjian wrote:
> Alex Hunsaker wrote:
> > On Wed, Apr 16, 2008 at 4:54 PM, Alvaro Herrera
> > <alvherre(at)commandprompt(dot)com> wrote:
> > > Joshua D. Drake escribi?:
> > >
> > > > That is an interesting idea. Something like:
> > > >
> > > > pg_restore -E "SET STATEMENT_TIMEOUT=0; SET MAINTENANCE_WORK_MEM=1G" ?
> > >
> > > We already have it -- it's called PGOPTIONS.
> > >
> >
> > Ok but is not the purpose of the patch to turn off statement_timeout
> > by *default* in pg_restore/pg_dump?
> >
> > Here is an updated patch for I posted above (with the command line
> > option --use-statement-timeout) for pg_dump and pg_restore.
>
> I would like to get do this without adding a new --use-statement-timeout
> flag. Is anyone going to want to honor statement_timeout during
> pg_dump/pg_restore? I thought we were just going to disable it.

I have a patch in the queue to use set statement timeout while pg_dump is
taking locks to avoid pg_dump hanging for other long running transactions
that may have done ddl. Do I need to repost for discussion now?

-dg

--
David Gould daveg(at)sonic(dot)net 510 536 1443 510 282 0869
If simplicity worked, the world would be overrun with insects.

daveg wrote:
> On Mon, Jun 23, 2008 at 06:51:28PM -0400, Bruce Momjian wrote:
> > Alex Hunsaker wrote:
> > > On Wed, Apr 16, 2008 at 4:54 PM, Alvaro Herrera
> > > <alvherre(at)commandprompt(dot)com> wrote:
> > > > Joshua D. Drake escribi?:
> > > >
> > > > > That is an interesting idea. Something like:
> > > > >
> > > > > pg_restore -E "SET STATEMENT_TIMEOUT=0; SET MAINTENANCE_WORK_MEM=1G" ?
> > > >
> > > > We already have it -- it's called PGOPTIONS.
> > > >
> > >
> > > Ok but is not the purpose of the patch to turn off statement_timeout
> > > by *default* in pg_restore/pg_dump?
> > >
> > > Here is an updated patch for I posted above (with the command line
> > > option --use-statement-timeout) for pg_dump and pg_restore.
> >
> > I would like to get do this without adding a new --use-statement-timeout
> > flag. Is anyone going to want to honor statement_timeout during
> > pg_dump/pg_restore? I thought we were just going to disable it.
>
> I have a patch in the queue to use set statement timeout while pg_dump is
> taking locks to avoid pg_dump hanging for other long running transactions
> that may have done ddl. Do I need to repost for discussion now?

I see it now, but I forgot how it would interact with this patch. We
would have to prevent --use-statement-timeout when lock timeout was
being used, but my point is that I see no value in having
--use-statement-timeout.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

On Mon, Jun 23, 2008 at 4:51 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> I would like to get do this without adding a new --use-statement-timeout
> flag. Is anyone going to want to honor statement_timeout during
> pg_dump/pg_restore? I thought we were just going to disable it.

I believe so. This was when not everyone was convinced. Im fairly
certain Josh original patch is in the commit fest. So feel free to
drop this one.

Alex Hunsaker wrote:
> On Mon, Jun 23, 2008 at 4:51 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> > I would like to get do this without adding a new --use-statement-timeout
> > flag. Is anyone going to want to honor statement_timeout during
> > pg_dump/pg_restore? I thought we were just going to disable it.
>
> I believe so. This was when not everyone was convinced. Im fairly
> certain Josh original patch is in the commit fest. So feel free to
> drop this one.

I certainly don't see any version of Drake's patch in the July
commitfest:

http://wiki.postgresql.org/wiki/CommitFest

I am thinking I will just remove the option and commit it.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Alex Hunsaker wrote:
> On Mon, Jun 23, 2008 at 4:51 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
>> I would like to get do this without adding a new --use-statement-timeout
>> flag. Is anyone going to want to honor statement_timeout during
>> pg_dump/pg_restore? I thought we were just going to disable it.
>
> I believe so. This was when not everyone was convinced. Im fairly
> certain Josh original patch is in the commit fest. So feel free to
> drop this one.
>

My patch has been committed.

Joshua D. Drake

Joshua D. Drake wrote:
> Alex Hunsaker wrote:
> > On Mon, Jun 23, 2008 at 4:51 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> >> I would like to get do this without adding a new --use-statement-timeout
> >> flag. Is anyone going to want to honor statement_timeout during
> >> pg_dump/pg_restore? I thought we were just going to disable it.
> >
> > I believe so. This was when not everyone was convinced. Im fairly
> > certain Josh original patch is in the commit fest. So feel free to
> > drop this one.
> >
>
> My patch has been committed.

Ah, I see, but with no switch. Thanks.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

On Mon, Jun 23, 2008 at 07:30:53PM -0400, Bruce Momjian wrote:
> daveg wrote:
> > On Mon, Jun 23, 2008 at 06:51:28PM -0400, Bruce Momjian wrote:
> > > Alex Hunsaker wrote:
> > > > On Wed, Apr 16, 2008 at 4:54 PM, Alvaro Herrera
> > > > <alvherre(at)commandprompt(dot)com> wrote:
> > > > > Joshua D. Drake escribi?:
> > > > >
> > > > > > That is an interesting idea. Something like:
> > > > > >
> > > > > > pg_restore -E "SET STATEMENT_TIMEOUT=0; SET MAINTENANCE_WORK_MEM=1G" ?
> > > > >
> > > > > We already have it -- it's called PGOPTIONS.
> > > > >
> > > >
> > > > Ok but is not the purpose of the patch to turn off statement_timeout
> > > > by *default* in pg_restore/pg_dump?
> > > >
> > > > Here is an updated patch for I posted above (with the command line
> > > > option --use-statement-timeout) for pg_dump and pg_restore.
> > >
> > > I would like to get do this without adding a new --use-statement-timeout
> > > flag. Is anyone going to want to honor statement_timeout during
> > > pg_dump/pg_restore? I thought we were just going to disable it.
> >
> > I have a patch in the queue to use set statement timeout while pg_dump is
> > taking locks to avoid pg_dump hanging for other long running transactions
> > that may have done ddl. Do I need to repost for discussion now?
>
> I see it now, but I forgot how it would interact with this patch. We
> would have to prevent --use-statement-timeout when lock timeout was
> being used, but my point is that I see no value in having
> --use-statement-timeout.

lock-timeout sets statement_timeout to a small value while locks are being
taken on all the tables. Then it resets it to default. So it could reset it
to whatever the new default is.

Do I need to adjust my patch or something?

-dg

--
David Gould daveg(at)sonic(dot)net 510 536 1443 510 282 0869
If simplicity worked, the world would be overrun with insects.

daveg <daveg(at)sonic(dot)net> writes:
> lock-timeout sets statement_timeout to a small value while locks are being
> taken on all the tables. Then it resets it to default. So it could reset it
> to whatever the new default is.

"reset to default" is *surely* not the right behavior; resetting to the
setting that had been in effect might be a bit sane. But the whole
design sounds pretty broken to start with. The timer management code
already understands the concept of scheduling the interrupt for the
nearest of a couple of potentially active timeouts. ISTM any patch
intended to add a feature like this ought to extend that logic rather
than hack around changing the values of global variables.

regards, tom lane

On Tue, Jun 24, 2008 at 05:34:50PM -0400, Tom Lane wrote:
> daveg <daveg(at)sonic(dot)net> writes:
> > lock-timeout sets statement_timeout to a small value while locks are being
> > taken on all the tables. Then it resets it to default. So it could reset it
> > to whatever the new default is.
>
> "reset to default" is *surely* not the right behavior; resetting to the
> setting that had been in effect might be a bit sane. But the whole
> design sounds pretty broken to start with. The timer management code
> already understands the concept of scheduling the interrupt for the
> nearest of a couple of potentially active timeouts. ISTM any patch
> intended to add a feature like this ought to extend that logic rather
> than hack around changing the values of global variables.

Are we talking about the same patch? Because I don't know what you are
refering to with "timer management code" and "scheduling the interrupt" in
the context of pg_dump.

-dg

--
David Gould daveg(at)sonic(dot)net 510 536 1443 510 282 0869
If simplicity worked, the world would be overrun with insects.

daveg <daveg(at)sonic(dot)net> writes:
> Are we talking about the same patch?

Maybe not --- I thought you were talking about a backend-side behavioral
change.

> Because I don't know what you are
> refering to with "timer management code" and "scheduling the interrupt" in
> the context of pg_dump.

I'm not sure that I see a good argument for making pg_dump deliberately
fail, if that's what you're proposing. Maybe I'm just too old-school,
but there simply is not any other higher priority for a database than
safeguarding your data.

regards, tom lane

On Tue, Jun 24, 2008 at 10:41:07PM -0400, Tom Lane wrote:
> daveg <daveg(at)sonic(dot)net> writes:
> > Are we talking about the same patch?
>
> Maybe not --- I thought you were talking about a backend-side behavioral
> change.
>
> > Because I don't know what you are
> > refering to with "timer management code" and "scheduling the interrupt" in
> > the context of pg_dump.
>
> I'm not sure that I see a good argument for making pg_dump deliberately
> fail, if that's what you're proposing. Maybe I'm just too old-school,
> but there simply is not any other higher priority for a database than
> safeguarding your data.

We agree about that. The intent of my patch it to give the user a chance to
take corrective action in a case where pg_dump cannot be relied on to succeed.

The problem is that pg_dump can get blocked behind locks and then fail hours
later when the locks are released because some table it had not locked yet
changed. In the worst case:

- no backup,
- no notice until too late to restart the backup,
- lost production due to other processes waiting on locks pg_dump holds.

So the intent of the patch is to optionally allow pg_dump to fail quickly
if it cannot get all the access share locks it needs. This gives the user
an opportunity to notice and retry in a timely way.

Please see http://archives.postgresql.org/pgsql-patches/2008-05/msg00351.php
for the orginal patch and problem description.

A sample failure instance from a very heavy batch environment with a lot of
materialized views being maintained concurrently with pg_dump. DB size
is about 300 GB:

---
20080410 14:53:49 dumpdb c04_20080410_public: dumping c04 to /backups/c04_20080410_public
pg_dump: SQL command failed
pg_dump: Error message from server: ERROR: cache lookup failed for index 22619852
pg_dump: The command was: SELECT t.tableoid, t.oid, t.relname as indexname, pg_catalog.pg_get_indexdef(i.indexrelid) as indexdef, t.relnatts as indnkeys, i.indkey, i.indisclustered, c.contype, c.conname, c.tableoid as contableoid, c.oid as conoid, (SELECT spcname FROM pg_catalog.pg_tablespace s WHERE s.oid = t.reltablespace) as tablespace, array_to_string(t.reloptions, ', ') as options FROM pg_catalog.pg_index i JOIN pg_catalog.pg_class t ON (t.oid = i.indexrelid) LEFT JOIN pg_catalog.pg_depend d ON (d.classid = t.tableoid AND d.objid = t.oid AND d.deptype = 'i') LEFT JOIN pg_catalog.pg_constraint c ON (d.refclassid = c.tableoid AND d.refobjid = c.oid) WHERE i.indrelid = '22615005'::pg_catalog.oid ORDER BY indexname
20080411 06:12:17 dumpdb FATAL: c04_20080410_public: dump failed
---

Note that the dump started at 14:53, but did not fail until 06:12 the next day,
and it never got to the actual copy out phase. Meanwhile other DDL using
processes were hung on the access share locks aready held by pg_dump.

Regards

-dg

--
David Gould daveg(at)sonic(dot)net 510 536 1443 510 282 0869
If simplicity worked, the world would be overrun with insects.