Patch for Prevent pg_dump/pg_restore from being affected by statement_timeout

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

On Tue, 11 Mar 2008 10:46:23 -0700
"Joshua D. Drake" <jd@commandprompt.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@commandprompt.com> wrote:

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

On Tue, 11 Mar 2008 10:46:23 -0700
"Joshua D. Drake" <jd@commandprompt.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@commandprompt.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@enterprisedb.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@enterprisedb.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@enterprisedb.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]Short of editing a potentially GB-size file, or using some sed/shell shenanigans to strip out the suspect setting.. 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.
some sed/shell shenanigans to strip out the suspect setting.

- --
Greg Sabino Mullane greg@turnstep.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@turnstep.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@commandprompt.com> wrote:

On Wed, 16 Apr 2008 22:17:30 -0000
"Greg Sabino Mullane" <greg@turnstep.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@commandprompt.com> wrote:

On Wed, 16 Apr 2008 15:22:31 -0700

"Joshua D. Drake" <jd@commandprompt.com> wrote:

On Wed, 16 Apr 2008 22:17:30 -0000
"Greg Sabino Mullane" <greg@turnstep.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@dunslane.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@dunslane.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@commandprompt.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@gmx.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