Thread: parallel restore fixes
The attached patch fixes two issues with parallel restore:
* the static buffer problem in dumputils.c::fmtId() on Windows
(solution: use thread-local storage)
* ReopenPtr() is called too often
There is one remaining bug I know of that I can reproduce: we can get
into deadlock when two tables are foreign keyed to each other. So I need
to get a bit more paranoid about dependencies.
I can't reproduce Olivier Prennant's file closing problem on Unixware.
Is it still happening after application of this patch?
cheers
andrew
Index: src/bin/pg_dump/dumputils.c
===================================================================
RCS file: /home/cvsmirror/pg/pgsql/src/bin/pg_dump/dumputils.c,v
retrieving revision 1.44
diff -c -r1.44 dumputils.c
*** src/bin/pg_dump/dumputils.c 22 Jan 2009 20:16:07 -0000 1.44
--- src/bin/pg_dump/dumputils.c 9 Mar 2009 22:33:32 -0000
***************
*** 31,36 ****
--- 31,50 ----
static void AddAcl(PQExpBuffer aclbuf, const char *keyword,
const char *subname);
+ #ifdef WIN32
+ static DWORD tls_index;
+ #endif
+
+ void
+ init_dump_utils()
+ {
+ #ifdef WIN32
+ /* reserve one thread-local slot for the fmtId query buffer address */
+ tls_index = TlsAlloc();
+ #endif
+ }
+
+
/*
* Quotes input string if it's not a legitimate SQL identifier as-is.
***************
*** 42,55 ****
--- 56,87 ----
const char *
fmtId(const char *rawid)
{
+ #ifdef WIN32
+ PQExpBuffer id_return;
+ #else
static PQExpBuffer id_return = NULL;
+ #endif
const char *cp;
bool need_quotes = false;
+ char *retval;
+
+ #ifdef WIN32
+ id_return = (PQExpBuffer) TlsGetValue(tls_index); /* returns 0 until set */
+ #endif
if (id_return) /* first time through? */
+ {
+ /* same buffer, just wipe contents */
resetPQExpBuffer(id_return);
+ }
else
+ {
+ /* new buffer */
id_return = createPQExpBuffer();
+ #ifdef WIN32
+ TlsSetValue(tls_index,id_return);
+ #endif
+ }
/*
* These checks need to match the identifier production in scan.l. Don't
***************
*** 111,117 ****
appendPQExpBufferChar(id_return, '\"');
}
! return id_return->data;
}
--- 143,151 ----
appendPQExpBufferChar(id_return, '\"');
}
! retval = id_return->data;
! return retval;
!
}
Index: src/bin/pg_dump/dumputils.h
===================================================================
RCS file: /home/cvsmirror/pg/pgsql/src/bin/pg_dump/dumputils.h,v
retrieving revision 1.23
diff -c -r1.23 dumputils.h
*** src/bin/pg_dump/dumputils.h 22 Jan 2009 20:16:07 -0000 1.23
--- src/bin/pg_dump/dumputils.h 9 Mar 2009 22:33:32 -0000
***************
*** 19,24 ****
--- 19,25 ----
#include "libpq-fe.h"
#include "pqexpbuffer.h"
+ extern void init_dump_utils(void);
extern const char *fmtId(const char *identifier);
extern void appendStringLiteral(PQExpBuffer buf, const char *str,
int encoding, bool std_strings);
Index: src/bin/pg_dump/pg_backup_archiver.c
===================================================================
RCS file: /home/cvsmirror/pg/pgsql/src/bin/pg_dump/pg_backup_archiver.c,v
retrieving revision 1.165
diff -c -r1.165 pg_backup_archiver.c
*** src/bin/pg_dump/pg_backup_archiver.c 5 Mar 2009 14:51:10 -0000 1.165
--- src/bin/pg_dump/pg_backup_archiver.c 9 Mar 2009 22:33:32 -0000
***************
*** 3467,3478 ****
/*
* Close and reopen the input file so we have a private file pointer
! * that doesn't stomp on anyone else's file pointer.
*
! * Note: on Windows, since we are using threads not processes, this
! * *doesn't* close the original file pointer but just open a new one.
*/
! (AH->ReopenPtr) (AH);
/*
* We need our own database connection, too
--- 3467,3486 ----
/*
* Close and reopen the input file so we have a private file pointer
! * that doesn't stomp on anyone else's file pointer, if we're actually
! * going to need to read from the file. Otherwise, just close it
! * except on Windows, where it will possibly be needed by other threads.
*
! * Note: on Windows, since we are using threads not processes, the
! * reopen call *doesn't* close the original file pointer but just open
! * a new one.
*/
! if (te->section == SECTION_DATA )
! (AH->ReopenPtr) (AH);
! #ifndef WIN32
! else
! (AH->ClosePtr) (AH);
! #endif;
/*
* We need our own database connection, too
***************
*** 3490,3495 ****
--- 3498,3505 ----
PQfinish(AH->connection);
AH->connection = NULL;
+ /* If we reopened the file, we are done with it, so close it now */
+ if (te->section == SECTION_DATA )
(AH->ClosePtr) (AH);
if (retval == 0 && AH->public.n_errors)
Index: src/bin/pg_dump/pg_dump.c
===================================================================
RCS file: /home/cvsmirror/pg/pgsql/src/bin/pg_dump/pg_dump.c,v
retrieving revision 1.528
diff -c -r1.528 pg_dump.c
*** src/bin/pg_dump/pg_dump.c 4 Mar 2009 11:57:00 -0000 1.528
--- src/bin/pg_dump/pg_dump.c 9 Mar 2009 22:33:32 -0000
***************
*** 287,292 ****
--- 287,294 ----
set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN("pg_dump"));
+ init_dump_utils();
+
g_verbose = false;
strcpy(g_comment_start, "-- ");
Index: src/bin/pg_dump/pg_dumpall.c
===================================================================
RCS file: /home/cvsmirror/pg/pgsql/src/bin/pg_dump/pg_dumpall.c,v
retrieving revision 1.119
diff -c -r1.119 pg_dumpall.c
*** src/bin/pg_dump/pg_dumpall.c 4 Mar 2009 11:57:00 -0000 1.119
--- src/bin/pg_dump/pg_dumpall.c 9 Mar 2009 22:33:32 -0000
***************
*** 136,141 ****
--- 136,143 ----
set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN("pg_dump"));
+ init_dump_utils();
+
progname = get_progname(argv[0]);
if (argc > 1)
Index: src/bin/pg_dump/pg_restore.c
===================================================================
RCS file: /home/cvsmirror/pg/pgsql/src/bin/pg_dump/pg_restore.c,v
retrieving revision 1.94
diff -c -r1.94 pg_restore.c
*** src/bin/pg_dump/pg_restore.c 26 Feb 2009 16:02:38 -0000 1.94
--- src/bin/pg_dump/pg_restore.c 9 Mar 2009 22:33:32 -0000
***************
*** 40,45 ****
--- 40,46 ----
*/
#include "pg_backup_archiver.h"
+ #include "dumputils.h"
#include <ctype.h>
***************
*** 125,130 ****
--- 126,133 ----
set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN("pg_dump"));
+ init_dump_utils();
+
opts = NewRestoreOptions();
progname = get_progname(argv[0]);
Andrew Dunstan wrote: > > The attached patch fixes two issues with parallel restore: > > * the static buffer problem in dumputils.c::fmtId() on Windows > (solution: use thread-local storage) > * ReopenPtr() is called too often Hmm, if pg_restore is the only program that's threaded, why are you calling init_dump_utils on pg_dump and pg_dumpall? It makes me a bit nervous because there are some other programs that are linking dumputils.c (psql and some in src/bin/scripts/) and even calling fmtId. Also I think the fmtId comment needs to be updated. -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Andrew Dunstan <andrew@dunslane.net> writes:
> + void
> + init_dump_utils()
This should be
> + void
> + init_dump_utils(void)
please. We don't do K&R C around here. I'd lose the added retval
variable too; that's not contributing anything.
> ! #endif;
Semicolon is bogus here.
Looks pretty sane otherwise.
regards, tom lane
Alvaro Herrera <alvherre@commandprompt.com> writes:
> Hmm, if pg_restore is the only program that's threaded, why are you
> calling init_dump_utils on pg_dump and pg_dumpall?
... because fmtId will crash on *any* use without that.
> It makes me a bit
> nervous because there are some other programs that are linking
> dumputils.c (psql and some in src/bin/scripts/) and even calling fmtId.
Actually, why bother with init_dump_utils at all? fmtId could be made
to initialize the ID variable for itself on first call, with only one
extra if-test, which is hardly gonna matter.
regards, tom lane
Tom Lane wrote: >> It makes me a bit >> nervous because there are some other programs that are linking >> dumputils.c (psql and some in src/bin/scripts/) and even calling fmtId. >> > > Actually, why bother with init_dump_utils at all? fmtId could be made > to initialize the ID variable for itself on first call, with only one > extra if-test, which is hardly gonna matter. > > Well, the Windows reference I have suggests TlsAlloc() needs to be called early in the initialisation process ... I guess I could force it with a dummy call to fmtId() in restore_toc_entries_parallel() before it starts spawning children, so we'd be sure there wasn't a race condition, and nothing else is going to have threads so it won't matter. We'd need a long comment to that effect, though ;-) > I'd lose the added retval > variable too; that's not contributing anything. It is, in fact. Until I put that in I was getting constant crashes. I suspect it's something to do with stuff Windows does under the hood on function return. cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> Tom Lane wrote:
>> Actually, why bother with init_dump_utils at all?
> Well, the Windows reference I have suggests TlsAlloc() needs to be
> called early in the initialisation process ...
How early is early? The proposed call sites for init_dump_utils seem
already long past the point where any libc-level infrastructure would
think it is "initialization" time.
>> I'd lose the added retval
>> variable too; that's not contributing anything.
> It is, in fact. Until I put that in I was getting constant crashes. I
> suspect it's something to do with stuff Windows does under the hood on
> function return.
Pardon me while I retrieve my eyebrows from the ceiling. I think you've
got something going on there you don't understand, and you need to
understand it not just put in a cargo-cult fix. (Especially one that's
not documented and hence likely to be removed by the next person who
touches the code.)
regards, tom lane
Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> writes: > >> Tom Lane wrote: >> >>> Actually, why bother with init_dump_utils at all? >>> > > >> Well, the Windows reference I have suggests TlsAlloc() needs to be >> called early in the initialisation process ... >> > > How early is early? The proposed call sites for init_dump_utils seem > already long past the point where any libc-level infrastructure would > think it is "initialization" time. > Well, I think at least it needs to be done where other threads won't be calling it at the same time. cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> Tom Lane wrote:
>> How early is early? The proposed call sites for init_dump_utils seem
>> already long past the point where any libc-level infrastructure would
>> think it is "initialization" time.
> Well, I think at least it needs to be done where other threads won't be
> calling it at the same time.
Oh, I see, ye olde race condition. Still, it seems like a bad idea
to expect that we will catch every program that might call fmtId;
as Alvaro notes, that's all over our client-side code.
How about this: by default, fmtId uses the same logic as now (one static
PQExpBuffer). If told to by a call of init_parallel_dump_utils(), which
need only be called by pg_restore during its startup, then it switches to
using per-thread storage. init_parallel_dump_utils can be the place
that calls TlsAlloc. This is almost the same as what you suggested a
couple messages back, but perhaps a bit clearer as to what's going on;
and it definitely cuts the number of places we need to touch.
regards, tom lane
Tom Lane wrote:
>
> How about this: by default, fmtId uses the same logic as now (one static
> PQExpBuffer). If told to by a call of init_parallel_dump_utils(), which
> need only be called by pg_restore during its startup, then it switches to
> using per-thread storage. init_parallel_dump_utils can be the place
> that calls TlsAlloc. This is almost the same as what you suggested a
> couple messages back, but perhaps a bit clearer as to what's going on;
> and it definitely cuts the number of places we need to touch.
>
>
>
OK, here 'tis.
Moving on to the deadlock with crossed FKs issue.
cheers
andrew
? src/bin/pg_dump/.deps
? src/bin/pg_dump/kwlookup.c
? src/bin/pg_dump/win32ver.rc
Index: src/bin/pg_dump/dumputils.c
===================================================================
RCS file: /home/cvsmirror/pg/pgsql/src/bin/pg_dump/dumputils.c,v
retrieving revision 1.44
diff -c -r1.44 dumputils.c
*** src/bin/pg_dump/dumputils.c 22 Jan 2009 20:16:07 -0000 1.44
--- src/bin/pg_dump/dumputils.c 10 Mar 2009 19:38:07 -0000
***************
*** 31,55 ****
static void AddAcl(PQExpBuffer aclbuf, const char *keyword,
const char *subname);
/*
! * Quotes input string if it's not a legitimate SQL identifier as-is.
*
! * Note that the returned string must be used before calling fmtId again,
! * since we re-use the same return buffer each time. Non-reentrant but
! * avoids memory leakage.
*/
const char *
fmtId(const char *rawid)
{
! static PQExpBuffer id_return = NULL;
const char *cp;
bool need_quotes = false;
if (id_return) /* first time through? */
resetPQExpBuffer(id_return);
else
id_return = createPQExpBuffer();
/*
* These checks need to match the identifier production in scan.l. Don't
--- 31,102 ----
static void AddAcl(PQExpBuffer aclbuf, const char *keyword,
const char *subname);
+ #ifdef WIN32
+ static bool parallel_init_done = false;
+ static DWORD tls_index;
+ #endif
+
+ void
+ init_parallel_dump_utils(void)
+ {
+ #ifdef WIN32
+ if (! parallel_init_done)
+ {
+ tls_index = TlsAlloc();
+ parallel_init_done = true;
+ }
+ #endif
+ }
/*
! * Quotes input string if it's not a legitimate SQL identifier as-is.
*
! * Note that the returned string must be used before calling fmtId again,
! * since we re-use the same return buffer each time. Non-reentrant but
! * reduces memory leakage. (On Windows the memory leakage will be one buffer
! * per thread, which is at least better than one per call).
*/
const char *
fmtId(const char *rawid)
{
! /*
! * The Tls code goes awry if we use a static var, so we provide for both
! * static and auto, and omit any use of the static var when using Tls.
! */
! static PQExpBuffer s_id_return = NULL;
! PQExpBuffer id_return;
!
const char *cp;
bool need_quotes = false;
+ #ifdef WIN32
+ if (parallel_init_done)
+ id_return = (PQExpBuffer) TlsGetValue(tls_index); /* 0 when not set */
+ else
+ id_return = s_id_return;
+ #else
+ id_return = s_id_return;
+ #endif
+
if (id_return) /* first time through? */
+ {
+ /* same buffer, just wipe contents */
resetPQExpBuffer(id_return);
+ }
else
+ {
+ /* new buffer */
id_return = createPQExpBuffer();
+ #ifdef WIN32
+ if (parallel_init_done)
+ TlsSetValue(tls_index,id_return);
+ else
+ s_id_return = id_return;
+ #else
+ s_id_return = id_return;
+ #endif
+
+ }
/*
* These checks need to match the identifier production in scan.l. Don't
Index: src/bin/pg_dump/dumputils.h
===================================================================
RCS file: /home/cvsmirror/pg/pgsql/src/bin/pg_dump/dumputils.h,v
retrieving revision 1.23
diff -c -r1.23 dumputils.h
*** src/bin/pg_dump/dumputils.h 22 Jan 2009 20:16:07 -0000 1.23
--- src/bin/pg_dump/dumputils.h 10 Mar 2009 19:38:07 -0000
***************
*** 19,24 ****
--- 19,25 ----
#include "libpq-fe.h"
#include "pqexpbuffer.h"
+ extern void init_parallel_dump_utils(void);
extern const char *fmtId(const char *identifier);
extern void appendStringLiteral(PQExpBuffer buf, const char *str,
int encoding, bool std_strings);
Index: src/bin/pg_dump/pg_backup_archiver.c
===================================================================
RCS file: /home/cvsmirror/pg/pgsql/src/bin/pg_dump/pg_backup_archiver.c,v
retrieving revision 1.165
diff -c -r1.165 pg_backup_archiver.c
*** src/bin/pg_dump/pg_backup_archiver.c 5 Mar 2009 14:51:10 -0000 1.165
--- src/bin/pg_dump/pg_backup_archiver.c 10 Mar 2009 19:38:07 -0000
***************
*** 3467,3478 ****
/*
* Close and reopen the input file so we have a private file pointer
! * that doesn't stomp on anyone else's file pointer.
*
! * Note: on Windows, since we are using threads not processes, this
! * *doesn't* close the original file pointer but just open a new one.
*/
! (AH->ReopenPtr) (AH);
/*
* We need our own database connection, too
--- 3467,3486 ----
/*
* Close and reopen the input file so we have a private file pointer
! * that doesn't stomp on anyone else's file pointer, if we're actually
! * going to need to read from the file. Otherwise, just close it
! * except on Windows, where it will possibly be needed by other threads.
*
! * Note: on Windows, since we are using threads not processes, the
! * reopen call *doesn't* close the original file pointer but just open
! * a new one.
*/
! if (te->section == SECTION_DATA )
! (AH->ReopenPtr) (AH);
! #ifndef WIN32
! else
! (AH->ClosePtr) (AH);
! #endif
/*
* We need our own database connection, too
***************
*** 3490,3496 ****
PQfinish(AH->connection);
AH->connection = NULL;
! (AH->ClosePtr) (AH);
if (retval == 0 && AH->public.n_errors)
retval = WORKER_IGNORED_ERRORS;
--- 3498,3506 ----
PQfinish(AH->connection);
AH->connection = NULL;
! /* If we reopened the file, we are done with it, so close it now */
! if (te->section == SECTION_DATA )
! (AH->ClosePtr) (AH);
if (retval == 0 && AH->public.n_errors)
retval = WORKER_IGNORED_ERRORS;
Index: src/bin/pg_dump/pg_restore.c
===================================================================
RCS file: /home/cvsmirror/pg/pgsql/src/bin/pg_dump/pg_restore.c,v
retrieving revision 1.94
diff -c -r1.94 pg_restore.c
*** src/bin/pg_dump/pg_restore.c 26 Feb 2009 16:02:38 -0000 1.94
--- src/bin/pg_dump/pg_restore.c 10 Mar 2009 19:38:07 -0000
***************
*** 40,45 ****
--- 40,46 ----
*/
#include "pg_backup_archiver.h"
+ #include "dumputils.h"
#include <ctype.h>
***************
*** 125,130 ****
--- 126,133 ----
set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN("pg_dump"));
+ init_parallel_dump_utils();
+
opts = NewRestoreOptions();
progname = get_progname(argv[0]);
Andrew Dunstan <andrew@dunslane.net> writes:
> OK, here 'tis.
Looks fairly reasonable to me, but of course I haven't tested it.
regards, tom lane
Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> writes: >> OK, here 'tis. > > Looks fairly reasonable to me, but of course I haven't tested it. Well, I have to do a blitz of parallel restores next week, so hopefully that will hit any soft spots. --Josh
Josh Berkus wrote: > Tom Lane wrote: >> Andrew Dunstan <andrew@dunslane.net> writes: >>> OK, here 'tis. >> >> Looks fairly reasonable to me, but of course I haven't tested it. > > Well, I have to do a blitz of parallel restores next week, so > hopefully that will hit any soft spots. I have a known outstanding bug to do with deadlock from FKs that cross (i.e. A has an FK that references B, and B has an FK that references A). The solution to this could be mildly complex, but I have an outline of it in my head. Workaround: recreate the failed FK at the end of the restore. The only other reported problem is the one on Unixware to do with closing the archive. I haven't been able to reproduce it on Linux or Windows, the two platforms I test on, although it might be fixed by the patch I just applied. cheers andrew