pg_dump lock timeout
Attached is a patch to add a commandline option to pg_dump to limit how long
pg_dump will wait for locks during startup.
The intent of this patch is to allow pg_dump to fail if a table lock cannot
be taken in a reasonable time. This allows the caller of pg_dump to retry or
otherwise correct the situation, without having locks held for long periods,
and without pg_dump having a long window during which catalog changes can
occur.
It works by setting statement_timeout to the user specified delay during
the startup phase where it is taking access share locks on all the tables.
Once all the locks are taken, it sets statement_timeout back to the default.
If a lock table statement times out, the dump fails with the statement timed
out error.
The orginal motivation was a client who runs heavy batch workloads and uses
truncate table and other DML in long transactions. This has created some
unhappy interaction scenarios with pg_dump:
- pg_dump ends up waiting hours on a DML table lock that is part of a long
transaction. Once the lock is released, pg_dump runs only to find
some table later in the list has been dropped. So pg_dump fails.
- pg_dump waits on a lock while holding access share locks on most of the
tables. Other processes that want to do DML wait on pg_dump. After a
while, large parts of the application are blocked while pg_dump waits
on locks. Eventually the operations staff notice that pg_dump is
blocking production and kill the dump.
Please have a look and consider it for merging.
Thanks
-dg
--
David Gould
If simplicity worked, the world would be overrun with insects.
On Sun, May 11, 2008 at 04:30:47AM -0700, daveg wrote:
Attached is a patch to add a commandline option to pg_dump to limit how long
pg_dump will wait for locks during startup.
Ooops, really attached this time.
-dg
--
David Gould daveg@sonic.net 510 536 1443 510 282 0869
If simplicity worked, the world would be overrun with insects.
Attachments:
pg_dump.c.timeout_patchtext/plain; charset=us-asciiDownload
*** pgsql/src/bin/pg_dump/pg_dump.c.orig 2008-05-11 03:23:06.000000000 -0700
--- pgsql/src/bin/pg_dump/pg_dump.c 2008-05-11 03:44:58.000000000 -0700
***************
*** 71,76 ****
--- 71,77 ----
bool schemaOnly;
bool dataOnly;
bool aclsSkip;
+ const char *lockWaitTimeout;
/* subquery used to convert user ID (eg, datdba) to user name */
static const char *username_subquery;
***************
*** 238,243 ****
--- 239,245 ----
{"column-inserts", no_argument, NULL, 'D'},
{"host", required_argument, NULL, 'h'},
{"ignore-version", no_argument, NULL, 'i'},
+ {"lock-wait-timeout", required_argument, NULL, 'l'},
{"no-reconnect", no_argument, NULL, 'R'},
{"oids", no_argument, NULL, 'o'},
{"no-owner", no_argument, NULL, 'O'},
***************
*** 278,283 ****
--- 280,286 ----
strcpy(g_opaque_type, "opaque");
dataOnly = schemaOnly = dumpInserts = attrNames = false;
+ lockWaitTimeout = NULL;
progname = get_progname(argv[0]);
***************
*** 299,305 ****
}
}
! while ((c = getopt_long(argc, argv, "abcCdDE:f:F:h:in:N:oOp:RsS:t:T:U:vWxX:Z:",
long_options, &optindex)) != -1)
{
switch (c)
--- 302,308 ----
}
}
! while ((c = getopt_long(argc, argv, "abcCdDE:f:F:h:il:n:N:oOp:RsS:t:T:U:vWxX:Z:",
long_options, &optindex)) != -1)
{
switch (c)
***************
*** 350,355 ****
--- 353,362 ----
/* ignored, deprecated option */
break;
+ case 'l': /* lock wait time */
+ lockWaitTimeout = optarg;
+ break;
+
case 'n': /* include schema(s) */
simple_string_list_append(&schema_include_patterns, optarg);
include_everything = false;
***************
*** 755,760 ****
--- 762,769 ----
printf(_("\nGeneral options:\n"));
printf(_(" -f, --file=FILENAME output file name\n"));
printf(_(" -F, --format=c|t|p output file format (custom, tar, plain text)\n"));
+ printf(_(" -l, --lock-wait-timeout=DELAY\n"
+ " timeout and fail after delay waiting for a table share lock\n"));
printf(_(" -v, --verbose verbose mode\n"));
printf(_(" -Z, --compress=0-9 compression level for compressed formats\n"));
printf(_(" --help show this help, then exit\n"));
***************
*** 3191,3196 ****
--- 3200,3213 ----
i_reltablespace = PQfnumber(res, "reltablespace");
i_reloptions = PQfnumber(res, "reloptions");
+ if (lockWaitTimeout)
+ {
+ /* Abandon the dump instead of waiting forever for a table lock */
+ resetPQExpBuffer(lockquery);
+ appendPQExpBuffer(lockquery, "SET statement_timeout = ");
+ appendStringLiteralConn(lockquery, lockWaitTimeout, g_conn);
+ do_sql_command(g_conn, lockquery->data);
+ }
for (i = 0; i < ntups; i++)
{
tblinfo[i].dobj.objType = DO_TABLE;
***************
*** 3259,3264 ****
--- 3276,3285 ----
tblinfo[i].dobj.name);
}
+ if (lockWaitTimeout)
+ {
+ do_sql_command(g_conn, "SET statement_timeout = default");
+ }
PQclear(res);
/*
pg_dump.sgml.timeout_patchtext/plain; charset=us-asciiDownload
*** pgsql/doc/src/sgml/ref/pg_dump.sgml.orig 2008-05-11 03:38:05.000000000 -0700
--- pgsql/doc/src/sgml/ref/pg_dump.sgml 2008-05-11 03:38:56.000000000 -0700
***************
*** 302,307 ****
--- 302,320 ----
</varlistentry>
<varlistentry>
+ <term><option>-l <replaceable class="parameter">wait_time</replaceable></option></term>
+ <term><option>--lock-wait-timeout=<replaceable class="parameter">wait_time</replaceable></option></term>
+ <listitem>
+ <para>
+ Do not wait forever for table locks at the start of the dump. Instead
+ time out and abandon the dump if unable to lock a table within the
+ specified wait time. The wait time is specified with the same formats
+ as accepted for intervals by the SET command.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
<term><option>-n <replaceable class="parameter">schema</replaceable></option></term>
<term><option>--schema=<replaceable class="parameter">schema</replaceable></option></term>
<listitem>
On Sun, May 11, 2008 at 06:00:35AM -0700, David Gould wrote:
On Sun, May 11, 2008 at 04:30:47AM -0700, daveg wrote:
Attached is a patch to add a commandline option to pg_dump to
limit how long pg_dump will wait for locks during startup.Ooops, really attached this time.
Can we see about getting this into the July commitfest? Dave has
presented a use case complete with logs where having this could have
prevented a failed backup and consequent data loss.
Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.com
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
Dave,
Just a few comments regarding your pg_dump lock timeout patch (in
general I like the concept and agree with adding it):
- No validity checking that the argument passed in has anything to do
with a number. The backend will do this, but it strikes me as a bit
odd to not do any checking at argument processing time.
- You call the argument 'wait time' in the documentation, but 'DELAY'
in the command-line help. I'd recommend using one term and sticking
to it. You're already two lines in the command-line help, you can
spell it out as 'WAIT_TIME' or similar.
- getTables() uses different variables for each query, and I'm
inclined to agree with that approach to make following the code
easier. I'd encourage you to add a new variable for the
statement_timeout query rather than reusing the lockqry variable.
You could even offset this by removing the unused delqry variable.
Otherwise, looks good to me.
Thanks,
Stephen
On 5/11/08, daveg <daveg@sonic.net> wrote:
Attached is a patch to add a commandline option to pg_dump to limit how long
pg_dump will wait for locks during startup.
My quick review:
- It does not seem important enough to waste a short option on.
Having only long option should be enough.
- It would be more polite to do SET LOCAL instead SET.
(Eg. it makes safer to use pg_dump through pooler.)
- The statement_timeout is set back with "statement_timeout = default"
Maybe it would be better to do "= 0" here? Although such decision
would go outside the scope of the patch, I see no sense having
any other statement_timeout for actual dumping.
--
marko
On Thu, Jul 03, 2008 at 11:15:10AM +0300, Marko Kreen wrote:
On 5/11/08, daveg <daveg@sonic.net> wrote:
Attached is a patch to add a commandline option to pg_dump to limit how long
pg_dump will wait for locks during startup.My quick review:
- It does not seem important enough to waste a short option on.
Having only long option should be enough.
Agreed. I'll change it.
- It would be more polite to do SET LOCAL instead SET.
(Eg. it makes safer to use pg_dump through pooler.)
Also agreed. Thanks.
- The statement_timeout is set back with "statement_timeout = default"
Maybe it would be better to do "= 0" here? Although such decision
would go outside the scope of the patch, I see no sense having
any other statement_timeout for actual dumping.
I'd prefer to leave whatever policy is otherwise in place alone. I can see
use cases for either having or not having a timeout for pg_dump, but it does
seem outside the scope of this patch.
Thanks for you review and comments.
-dg
--
David Gould daveg@sonic.net 510 536 1443 510 282 0869
If simplicity worked, the world would be overrun with insects.
daveg <daveg@sonic.net> writes:
On Thu, Jul 03, 2008 at 11:15:10AM +0300, Marko Kreen wrote:
- The statement_timeout is set back with "statement_timeout = default"
Maybe it would be better to do "= 0" here? Although such decision
would go outside the scope of the patch, I see no sense having
any other statement_timeout for actual dumping.
I'd prefer to leave whatever policy is otherwise in place alone.
The policy in place in CVS HEAD is that pg_dump explicitly sets
statement_timeout to 0. Setting it to default would break that,
and will certainly not be accepted.
regards, tom lane
On Thu, Jul 03, 2008 at 05:55:01AM -0700, daveg wrote:
On Thu, Jul 03, 2008 at 11:15:10AM +0300, Marko Kreen wrote:
On 5/11/08, daveg <daveg@sonic.net> wrote:
Attached is a patch to add a commandline option to pg_dump to limit how long
pg_dump will wait for locks during startup.My quick review:
- It does not seem important enough to waste a short option on.
Having only long option should be enough.Agreed. I'll change it.
- It would be more polite to do SET LOCAL instead SET.
(Eg. it makes safer to use pg_dump through pooler.)Also agreed. Thanks.
On second glance, pg_dump sets lots of variables without using SET LOCAL.
I think fixing that must be the subject of a separate patch as fixing just
one of many will only cause confusion.
- The statement_timeout is set back with "statement_timeout = default"
Maybe it would be better to do "= 0" here? Although such decision
would go outside the scope of the patch, I see no sense having
any other statement_timeout for actual dumping.I'd prefer to leave whatever policy is otherwise in place alone. I can see
use cases for either having or not having a timeout for pg_dump, but it does
seem outside the scope of this patch.
As it happens, another patch has set the policy to "statement_timeout = 0",
so I will follow that.
I'm sending in the revised patch today.
-dg
--
David Gould daveg@sonic.net 510 536 1443 510 282 0869
If simplicity worked, the world would be overrun with insects.
Here is an updated version of this patch against head. It builds, runs and
functions as expected. I did not build the sgml.
I've made changes based on various comments as follows:
- use WAIT_TIME in description consistantly. Reworded for clarity.
(Stephan Frost)
- Use a separate query buffer in getTables() (Stephan Frost)
- sets statement_timeout=0 afterwards per new policy (Tom Lane, Marko Kreen)
- has only --long-option to conserve short option letters (Marko Kreen)
Regards
-dg
--
David Gould daveg@sonic.net 510 536 1443 510 282 0869
If simplicity worked, the world would be overrun with insects.
Attachments:
pg-dump-timeout.patchtext/plain; charset=us-asciiDownload
*** a/doc/src/sgml/ref/pg_dump.sgml
--- b/doc/src/sgml/ref/pg_dump.sgml
***************
*** 557,564 **** PostgreSQL documentation
This option disables the use of dollar quoting for function bodies,
and forces them to be quoted using SQL standard string syntax.
</para>
! </listitem>
! </varlistentry>
<varlistentry>
<term><option>--disable-triggers</></term>
--- 557,564 ----
This option disables the use of dollar quoting for function bodies,
and forces them to be quoted using SQL standard string syntax.
</para>
! </listitem>
! </varlistentry>
<varlistentry>
<term><option>--disable-triggers</></term>
***************
*** 588,593 **** PostgreSQL documentation
--- 588,605 ----
</varlistentry>
<varlistentry>
+ <term><option>--lock-wait-timeout=<replaceable class="parameter">wait_time</replaceable></option></term>
+ <listitem>
+ <para>
+ Do not wait forever for table locks at the start of the dump. Instead
+ time out and abandon the dump if unable to lock a table within the
+ specified wait time. The wait time may be specified with the same
+ formats as accepted by <command>SET statement_timeout</>.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
<term><option>--use-set-session-authorization</></term>
<listitem>
<para>
*** a/src/bin/pg_dump/pg_dump.c
--- b/src/bin/pg_dump/pg_dump.c
***************
*** 71,76 **** bool attrNames; /* put attr names into insert strings */
--- 71,77 ----
bool schemaOnly;
bool dataOnly;
bool aclsSkip;
+ const char *lockWaitTimeout;
/* subquery used to convert user ID (eg, datdba) to user name */
static const char *username_subquery;
***************
*** 265,270 **** main(int argc, char **argv)
--- 266,275 ----
{"disable-triggers", no_argument, &disable_triggers, 1},
{"no-tablespaces", no_argument, &outputNoTablespaces, 1},
{"use-set-session-authorization", no_argument, &use_setsessauth, 1},
+ /*
+ * long options with arguments and no short option letter
+ */
+ {"lock-wait-timeout", required_argument, NULL, 1},
{NULL, 0, NULL, 0}
};
***************
*** 278,283 **** main(int argc, char **argv)
--- 283,289 ----
strcpy(g_opaque_type, "opaque");
dataOnly = schemaOnly = dumpInserts = attrNames = false;
+ lockWaitTimeout = NULL;
progname = get_progname(argv[0]);
***************
*** 436,441 **** main(int argc, char **argv)
--- 442,452 ----
/* This covers the long options equivalent to -X xxx. */
break;
+ case 1:
+ /* lock-wait-timeout */
+ lockWaitTimeout = optarg;
+ break;
+
default:
fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
exit(1);
***************
*** 757,762 **** help(const char *progname)
--- 768,776 ----
printf(_(" -F, --format=c|t|p output file format (custom, tar, plain text)\n"));
printf(_(" -v, --verbose verbose mode\n"));
printf(_(" -Z, --compress=0-9 compression level for compressed formats\n"));
+ printf(_(" --lock-wait-timeout=WAIT_TIME\n"
+ " timeout and fail after waiting WAIT_TIME\n"
+ " for a table lock during startup\n"));
printf(_(" --help show this help, then exit\n"));
printf(_(" --version output version information, then exit\n"));
***************
*** 2956,2962 **** getTables(int *numTables)
int ntups;
int i;
PQExpBuffer query = createPQExpBuffer();
! PQExpBuffer delqry = createPQExpBuffer();
PQExpBuffer lockquery = createPQExpBuffer();
TableInfo *tblinfo;
int i_reltableoid;
--- 2970,2976 ----
int ntups;
int i;
PQExpBuffer query = createPQExpBuffer();
! PQExpBuffer waitquery = createPQExpBuffer();
PQExpBuffer lockquery = createPQExpBuffer();
TableInfo *tblinfo;
int i_reltableoid;
***************
*** 3191,3196 **** getTables(int *numTables)
--- 3205,3218 ----
i_reltablespace = PQfnumber(res, "reltablespace");
i_reloptions = PQfnumber(res, "reloptions");
+ if (lockWaitTimeout)
+ {
+ /* Abandon the dump instead of waiting forever for a table lock */
+ resetPQExpBuffer(waitquery);
+ appendPQExpBuffer(waitquery, "SET statement_timeout = ");
+ appendStringLiteralConn(waitquery, lockWaitTimeout, g_conn);
+ do_sql_command(g_conn, waitquery->data);
+ }
for (i = 0; i < ntups; i++)
{
tblinfo[i].dobj.objType = DO_TABLE;
***************
*** 3259,3264 **** getTables(int *numTables)
--- 3281,3290 ----
tblinfo[i].dobj.name);
}
+ if (lockWaitTimeout)
+ {
+ do_sql_command(g_conn, "SET statement_timeout = 0");
+ }
PQclear(res);
/*
***************
*** 3291,3297 **** getTables(int *numTables)
}
destroyPQExpBuffer(query);
! destroyPQExpBuffer(delqry);
destroyPQExpBuffer(lockquery);
return tblinfo;
--- 3317,3323 ----
}
destroyPQExpBuffer(query);
! destroyPQExpBuffer(waitquery);
destroyPQExpBuffer(lockquery);
return tblinfo;
daveg <daveg@sonic.net> writes:
Here is an updated version of this patch against head. It builds, runs and
functions as expected. I did not build the sgml.
Applied with mostly minor cosmetic improvements --- the only actual
error I noticed was failing to check whether the server version supports
statement_timeout.
In most cases our policy has been that pg_dumpall should accept and pass
through any pg_dump option for which it's sensible to do so. I did not
make that happen but it seems it'd be a reasonable follow-on patch.
A minor point is that the syntax "-X lock-wait-timeout=n" or
"-X lock-wait-timeout n" won't work, although perhaps people used to
-X might expect it to. Since we deprecate -X (and don't even document
it anymore), I thought that making this work would be much more trouble
than it's worth, but perhaps that's open to argument.
regards, tom lane
On Sun, Jul 20, 2008 at 02:50:50PM -0400, Tom Lane wrote:
daveg <daveg@sonic.net> writes:
Here is an updated version of this patch against head. It builds, runs and
functions as expected. I did not build the sgml.Applied with mostly minor cosmetic improvements --- the only actual
error I noticed was failing to check whether the server version supports
statement_timeout.
I chose not to test backend version on the grounds that getting an explicit
failure for an explicitly requested option would be preferable to it being
silently ignored. However if the user is trying to use the same scripts for
many versions then ignoring unsupported but unessential features may be
preferred.
One of the cosmetic changes made in response to other reviewers was to
not reuse lockquery, instead to have a separate query buffer. You have
reversed that and eliminated lockquery too. Which seems better.
In most cases our policy has been that pg_dumpall should accept and pass
through any pg_dump option for which it's sensible to do so. I did not
make that happen but it seems it'd be a reasonable follow-on patch.
I'll remember that next time.
A minor point is that the syntax "-X lock-wait-timeout=n" or
"-X lock-wait-timeout n" won't work, although perhaps people used to
-X might expect it to. Since we deprecate -X (and don't even document
it anymore), I thought that making this work would be much more trouble
than it's worth, but perhaps that's open to argument.
All the other -X options are flags and supported directly by the getopt
machinery. Adding a -X option with an argument would require parsing
the argument by hand including dealing with '=' or ' ' as the separator and
telling getopt you had eaten an extra argument. This seemed a bit too much
code for the value of supporting a deprecated format for a brand new option.
Finally, you changed the option value and case from 1 to case 2. getopt_long()
only returns the value argument when there is no flag to set, so 1 was unique
and could have been read as "the first no-short option with an argument".
Thanks for checking this in.
-dg
--
David Gould daveg@sonic.net 510 536 1443 510 282 0869
If simplicity worked, the world would be overrun with insects.
daveg <daveg@sonic.net> writes:
On Sun, Jul 20, 2008 at 02:50:50PM -0400, Tom Lane wrote:
In most cases our policy has been that pg_dumpall should accept and pass
through any pg_dump option for which it's sensible to do so. I did not
make that happen but it seems it'd be a reasonable follow-on patch.
I'll remember that next time.
Er .. actually that was a direct request for you to do it.
Finally, you changed the option value and case from 1 to case 2. getopt_long
only returns the value argument when there is no flag to set, so 1 was unique
and could have been read as "the first no-short option with an argument".
Yeah. The code *worked* as you submitted it, but what was bothering me
was that the "val = 1" table entries worked in two completely different
ways for the different argument types. I first thought that you'd
broken the existing long argument options --- you hadn't, but I had to
go re-read the getopt_long source to convince myself of that. So it
seemed like using a different "val" value might help clarify the
difference in behavior for future readers of the code. In particular
the next guy who wants to add a long option with parameter value would
certainly not be able to use val = 1; but I thought the code as you
had it wouldn't give him any clue what to do. If you've got a better
idea about how to deal with that, feel free...
regards, tom lane
On Mon, Jul 21, 2008 at 03:43:11AM -0400, Tom Lane wrote:
daveg <daveg@sonic.net> writes:
On Sun, Jul 20, 2008 at 02:50:50PM -0400, Tom Lane wrote:
In most cases our policy has been that pg_dumpall should accept and pass
through any pg_dump option for which it's sensible to do so. I did not
make that happen but it seems it'd be a reasonable follow-on patch.I'll remember that next time.
Er .. actually that was a direct request for you to do it.
Attached is a the followon patch for pg_dumpall and docs to match pg_dump.
On a second topic, is anyone working on a parallel dump/load? I'd be
interested in helping.
-dg
--
David Gould daveg@sonic.net 510 536 1443 510 282 0869
If simplicity worked, the world would be overrun with insects.
Attachments:
pg-dumpall-timeout.patchtext/plain; charset=us-asciiDownload
*** a/doc/src/sgml/ref/pg_dumpall.sgml
--- b/doc/src/sgml/ref/pg_dumpall.sgml
***************
*** 196,201 **** PostgreSQL documentation
--- 196,217 ----
</varlistentry>
<varlistentry>
+ <term><option>--lock-wait-timeout=<replaceable class="parameter">timeout</replaceable></option></term>
+ <listitem>
+ <para>
+ Do not wait forever to acquire shared table locks at the beginning of
+ the dump. Instead fail if unable to lock a table within the specified
+ <replaceable class="parameter">timeout</>. The timeout may be
+ specified in any of the formats accepted by <command>SET
+ statement_timeout</>. (Allowed values vary depending on the server
+ version you are dumping from, but an integer number of milliseconds
+ is accepted by all versions since 7.3. This option is ignored when
+ dumping from a pre-7.3 server.)
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
<term><option>--no-tablespaces</option></term>
<listitem>
<para>
*** a/src/bin/pg_dump/pg_dumpall.c
--- b/src/bin/pg_dump/pg_dumpall.c
***************
*** 120,125 **** main(int argc, char *argv[])
--- 120,126 ----
{"disable-triggers", no_argument, &disable_triggers, 1},
{"no-tablespaces", no_argument, &no_tablespaces, 1},
{"use-set-session-authorization", no_argument, &use_setsessauth, 1},
+ {"lock-wait-timeout", required_argument, NULL, 2},
{NULL, 0, NULL, 0}
};
***************
*** 305,310 **** main(int argc, char *argv[])
--- 306,316 ----
case 0:
break;
+ case 2:
+ appendPQExpBuffer(pgdumpopts, " --lock-wait-timeout=");
+ appendPQExpBuffer(pgdumpopts, optarg);
+ break;
+
default:
fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
exit(1);
***************
*** 488,493 **** help(void)
--- 494,500 ----
printf(_(" -f, --file=FILENAME output file name\n"));
printf(_(" --help show this help, then exit\n"));
printf(_(" --version output version information, then exit\n"));
+ printf(_(" --lock-wait-timeout=TIMEOUT fail after waiting TIMEOUT for a table lock\n"));
printf(_("\nOptions controlling the output content:\n"));
printf(_(" -a, --data-only dump only the data, not the schema\n"));
printf(_(" -c, --clean clean (drop) databases prior to create\n"));
daveg wrote:
Attached is a the followon patch for pg_dumpall and docs to match pg_dump.
Thanks, committed. (The only change I did was to correct the alignment
in help output.)
--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support