Re: small parallel restore optimization

Here's a little optimization for the parallel restore code, that
inhibits reopening the archive file unless the worker will actually need
to read from the file (i.e. a data member). It seems to work OK on both
Linux and Windows, and I propose to apply it in a day or two.

I've seen a recent error that suggests we are clobbering memory
somewhere in the parallel code, as well as Olivier Prennant's reported
error that suggests the same thing, although I'm blessed if I can see
where it might be. Maybe some more eyeballs on the code would help.

cheers

andrew

Index: pg_backup_archiver.c
===================================================================
RCS file: /cvsroot/pgsql/src/bin/pg_dump/pg_backup_archiver.c,v
retrieving revision 1.165
diff -c -u -r1.165 pg_backup_archiver.c
cvs diff: conflicting specifications of output style
--- pg_backup_archiver.c 5 Mar 2009 14:51:10 -0000 1.165
+++ pg_backup_archiver.c 6 Mar 2009 16:51:41 -0000
@@ -3471,8 +3471,11 @@
*
* Note: on Windows, since we are using threads not processes, this
* *doesn't* close the original file pointer but just open a new
one.
+ *
+ * Only do this if we actually need to read the file for data.
*/
- (AH->ReopenPtr) (AH);
+ if ( te->section == SECTION_DATA )
+ (AH->ReopenPtr) (AH);

/*
* We need our own database connection, too
@@ -3490,7 +3493,9 @@
PQfinish(AH->connection);
AH->connection = NULL;

- (AH->ClosePtr) (AH);
+ /* close the file if we reopened it */
+ if ( te->section == SECTION_DATA )
+ (AH->ClosePtr) (AH);

if (retval == 0 && AH->public.n_errors)
retval = WORKER_IGNORED_ERRORS;

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> Here's a little optimization for the parallel restore code, that
> inhibits reopening the archive file unless the worker will actually need
> to read from the file (i.e. a data member). It seems to work OK on both
> Linux and Windows, and I propose to apply it in a day or two.

I think you should close the file immediately at fork if you're not
going to reopen it --- otherwise it's a foot-gun waiting to fire.
IOW, not this, but something more like

if (te->section == SECTION_DATA)
(AH->ReopenPtr) (AH);
else
(AH->ClosePtr) (AH);

... worker task ...

if (te->section == SECTION_DATA)
(AH->ClosePtr) (AH);

> I've seen a recent error that suggests we are clobbering memory
> somewhere in the parallel code, as well as Olivier Prennant's reported
> error that suggests the same thing, although I'm blessed if I can see
> where it might be. Maybe some more eyeballs on the code would help.

Can you put together even a weakly reproducible test case? Something
that only fails every tenth or hundredth time would still help.

regards, tom lane

On Fri, Mar 6, 2009 at 6:20 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Can you put together even a weakly reproducible test case?  Something
> that only fails every tenth or hundredth time would still help.

It seems that Olivier can reproduce the problem at will on Unixware. I
don't know if it's easy to find useful information to debug the
problem on this platform though.

See http://archives.postgresql.org/pgsql-hackers/2009-03/msg00201.php

--
Guillaume

On Fri, 6 Mar 2009, Guillaume Smet wrote:

> Date: Fri, 6 Mar 2009 18:58:58 +0100
> From: Guillaume Smet <guillaume(dot)smet(at)gmail(dot)com>
> To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
> Cc: Andrew Dunstan <andrew(at)dunslane(dot)net>,
> PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>, ohp(at)pyrenet(dot)fr
> Subject: Re: [HACKERS] small parallel restore optimization
>
> On Fri, Mar 6, 2009 at 6:20 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Can you put together even a weakly reproducible test case?  Something
>> that only fails every tenth or hundredth time would still help.
not sure, none of my tests did fail at the same place.
the only thing I could come with is a calloc(1,12) that seems to alloc
mem for filename, in that case sdewitte.dmp; so the alloc is not counting
the null char at the end.
not sure it could explain everything though
>
> It seems that Olivier can reproduce the problem at will on Unixware. I
> don't know if it's easy to find useful information to debug the
> problem on this platform though.
>
> See http://archives.postgresql.org/pgsql-hackers/2009-03/msg00201.php
>
>

--
Olivier PRENANT Tel: +33-5-61-50-97-00 (Work)
15, Chemin des Monges +33-5-61-50-97-01 (Fax)
31190 AUTERIVE +33-6-07-63-80-64 (GSM)
FRANCE Email: ohp(at)pyrenet(dot)fr
------------------------------------------------------------------------------
Make your life a dream, make your dream a reality. (St Exupery)
>From pgsql-hackers-owner(at)postgresql(dot)org Sat Mar 7 05:55:23 2009
Received: from localhost (unknown [200.46.204.183])
by mail.postgresql.org (Postfix) with ESMTP id 7F1E4632803
for <pgsql-hackers-postgresql(dot)org(at)mail(dot)postgresql(dot)org>; Sat, 7 Mar 2009 05:55:16 -0400 (AST)
Received: from mail.postgresql.org ([200.46.204.86])
by localhost (mx1.hub.org [200.46.204.183]) (amavisd-maia, port 10024)
with ESMTP id 90730-10
for <pgsql-hackers-postgresql(dot)org(at)mail(dot)postgresql(dot)org>;
Sat, 7 Mar 2009 05:55:09 -0400 (AST)
X-Greylist: from auto-whitelisted by SQLgrey-1.7.6
Received: from smtp.guitar.ocn.ne.jp (guitar.ocn.ne.jp [122.28.30.209])
by mail.postgresql.org (Postfix) with ESMTP id 0A51663219C
for <pgsql-hackers(at)postgresql(dot)org>; Sat, 7 Mar 2009 05:55:01 -0400 (AST)
Received: from HIRO57887DE653 (p5023-ipadfx01funabasi.chiba.ocn.ne.jp [218.43.105.23])
by smtp.guitar.ocn.ne.jp (Postfix) with SMTP
id 262E8278B; Sat, 7 Mar 2009 18:54:58 +0900 (JST)
Message-ID: <459F93FBB5474274BACADD11D25F16E9(at)HIRO57887DE653>
From: "Hiroshi Saito" <z-saito(at)guitar(dot)ocn(dot)ne(dot)jp>
To: "Peter Eisentraut" <peter_e(at)gmx(dot)net>,
"Alvaro Herrera" <alvherre(at)commandprompt(dot)com>
Cc: <pgsql-hackers(at)postgresql(dot)org>
References: <20090217113434(dot)439D77559ED(at)cvs(dot)postgresql(dot)org> <20090217162747(dot)GD3173(at)alvh(dot)no-ip(dot)org> <499BF4C8(dot)7000202(at)gmx(dot)net> <B931FEADCC0D4A1799C008E7C7D6777A(at)HIRO57887DE653>
Subject: Re: Re: [COMMITTERS] pgsql: Redefine _() to dgettext() instead of gettext() so that it uses
Date: Sat, 7 Mar 2009 18:54:56 +0900
MIME-Version: 1.0
Content-Type: multipart/mixed;
boundary="----=_NextPart_000_00C3_01C99F56.35140C80"
X-Priority: 3
X-MSMail-Priority: Normal
X-Mailer: Microsoft Outlook Express 6.00.2900.5512
X-MimeOLE: Produced By Microsoft MimeOLE V6.00.2900.5512
X-Virus-Scanned: Maia Mailguard 1.0.1
X-Spam-Status: No, hits=1.819 tagged_above=0 required=5
tests=MIME_QP_LONG_LINE=1.819
X-Spam-Level: *
X-Archive-Number: 200903/287
X-Sequence-Number: 134966

This is a multi-part message in MIME format.

------=_NextPart_000_00C3_01C99F56.35140C80
Content-Type: text/plain;
format=flowed;
charset="ISO-8859-1";
reply-type=original
Content-Transfer-Encoding: 7bit

Hi.

Ummm, I present is not understood by the reason of Japanese. ...
Therefore, It was made into the Spanish case.
I know that Alvaro-san will understand Spanish well.

==postgresql.conf==
lc_messages= 'es'

server.log of a patch before and after was applied.
Please see it.

Regards,
Hiroshi Saito

----- Original Message -----
From: "Hiroshi Saito" <z-saito(at)guitar(dot)ocn(dot)ne(dot)jp>

> Hi Peter-san.
>
> I see the problem for being an original domain in plpgsql. It differs from what
> codeset meant at postmaster by Japanese windows....
> Please see, this look at the problem on which SJIS enters into a message.
> http://winpg.jp/~saito/pg_work/LC_MESSAGE_CHECK/plpgsql/before_plpgsql_server.log
> This state is the following.
> ==
> lc_messages=ja
> server_encoding=utf-8
> ==
>
> Therefore, it needs to be codeset called for an original domain. It is the procedure in which
> only a server module must correspond. Then, It is solvable by this patch.
> http://winpg.jp/~saito/pg_work/LC_MESSAGE_CHECK/plpgsql/after_plpgsql_server.log
>
> Please take this into consideration.
> Tahnks.
>
> Regards,
> Hiroshi Saito
>
> ----- Original Message -----
> From: "Peter Eisentraut" <peter_e(at)gmx(dot)net>
>
>
>> Alvaro Herrera wrote:
>>> Peter Eisentraut wrote:
>>>> Log Message:
>>>> -----------
>>>> Redefine _() to dgettext() instead of gettext() so that it uses the plpgsql
>>>> text domain, instead of the postgres one (or whatever the default may be).
>>>
>>> Hmm, so is this needed on all other PLs too?
>>
>> In principle yes. Or call dgettext() explicitly, which is also done in
>> some cases. However, in most cases messages are issued through
>> ereport(), which handles this automatically (which you implemented, I
>> recall).
>
------=_NextPart_000_00C3_01C99F56.35140C80
Content-Type: application/octet-stream;
name="plpgsql_es_before.log"
Content-Transfer-Encoding: quoted-printable
Content-Disposition: attachment;
filename="plpgsql_es_before.log"

LOG: el sistema de bases de datos fue apagado en 2009-03-07 18:24:31 JST=0A=
LOG: el sistema de bases de datos est=81La listo para aceptar =
conexiones
LOG: lanzador de autovacuum iniciado=0A=
ERROR: un identificador entre comillas est=81La inconcluso=0A=
CONTEXTO: compilation of PL/pgSQL function "func1" near line 2=0A=
SENTENCIA: create function func1() returns int as '=0A=
begin=0A=
select "a;=0A=
end;=0A=
' language 'plpgsql';=0A=
ERROR: secuencia de bytes no v=C3=A1lida para codificaci=C3=B3n =
=C2=ABUTF8=C2=BB: 0x81=0A=
HINT: Este error tambi=C3=A9n puede ocurrir si la secuencia de bytes no =
coinciden con la codificaci=C3=B3n esperada por el servidor, lo cual es =
controlado por el par=C3=A1metro =C2=ABclient_encoding=C2=BB.=0A=
SENTENCIA: create function func1() returns int as '=0A=
begin=0A=
select "a;=0A=
end;=0A=
' language 'plpgsql';=0A=
LOG: se recibi=81Lo petici=81Lon de apagado inteligente
LOG: apagando lanzador de autovacuum=0A=
LOG: apagando=0A=
LOG: el sistema de bases de datos est=81La apagado=0A=

------=_NextPart_000_00C3_01C99F56.35140C80
Content-Type: application/octet-stream;
name="plpgsql_es_after.log"
Content-Transfer-Encoding: quoted-printable
Content-Disposition: attachment;
filename="plpgsql_es_after.log"

LOG: el sistema de bases de datos fue apagado en 2009-03-07 18:25:46 JST=0A=
LOG: el sistema de bases de datos est=81La listo para aceptar =
conexiones
LOG: lanzador de autovacuum iniciado=0A=
ERROR: un identificador entre comillas est=C3=A1 inconcluso=0A=
CONTEXTO: compilation of PL/pgSQL function "func1" near line 2=0A=
SENTENCIA: create function func1() returns int as '=0A=
begin=0A=
select "a;=0A=
end;=0A=
' language 'plpgsql';=0A=
LOG: se recibi=81Lo petici=81Lon de apagado inteligente
LOG: apagando lanzador de autovacuum=0A=
LOG: apagando=0A=
LOG: el sistema de bases de datos est=81La apagado=0A=

------=_NextPart_000_00C3_01C99F56.35140C80
Content-Type: application/octet-stream;
name="plpgsql_test.sql"
Content-Transfer-Encoding: quoted-printable
Content-Disposition: attachment;
filename="plpgsql_test.sql"

\encoding latin1=0A=
show client_encoding;=0A=
show server_encoding;=0A=
set lc_messages to es;=0A=
show lc_messages;=0A=
create function func1() returns int as '=0A=
begin=0A=
select "a;=0A=
end;=0A=
' language 'plpgsql';=0A=

------=_NextPart_000_00C3_01C99F56.35140C80--

ohp(at)pyrenet(dot)fr writes:
> the only thing I could come with is a calloc(1,12) that seems to alloc
> mem for filename, in that case sdewitte.dmp; so the alloc is not counting
> the null char at the end.

Where do you see that? The memtool dump you sent doesn't show it AFAICS.

regards, tom lane

Tom Lane wrote:
> ohp(at)pyrenet(dot)fr writes:
>
>> the only thing I could come with is a calloc(1,12) that seems to alloc
>> mem for filename, in that case sdewitte.dmp; so the alloc is not counting
>> the null char at the end.
>>
>
> Where do you see that? The memtool dump you sent doesn't show it AFAICS.
>
>
>

And the only copying of the filename that I can find is done with strdup().

cheers

andrew

Tom Lane wrote:
>> I've seen a recent error that suggests we are clobbering memory
>> somewhere in the parallel code, as well as Olivier Prennant's reported
>> error that suggests the same thing, although I'm blessed if I can see
>> where it might be. Maybe some more eyeballs on the code would help.
>>
>
> Can you put together even a weakly reproducible test case? Something
> that only fails every tenth or hundredth time would still help.
>
>
>

I have found the source of the problem I saw. dumputils.c:fmtId() uses a
static PQExpBuffer which it initialises the first time it's called. This
gets clobbered by simultaneous calls by Windows threads.

I could just make it auto and set it up on each call, but that could
result in a non-trivial memory leak ... it's probably called a great
many times. Or I could provide a parallel version where we pass in a
PQExpBuffer that we create, one per thread, and is used by anything
called by the parallel code. That seems like a bit of a potential
footgun, though.

Has anyone got a better plan?

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> I have found the source of the problem I saw. dumputils.c:fmtId() uses a
> static PQExpBuffer which it initialises the first time it's called. This
> gets clobbered by simultaneous calls by Windows threads.

Ugh. But that doesn't explain the original trouble report on Unixware.

> I could just make it auto and set it up on each call, but that could
> result in a non-trivial memory leak ... it's probably called a great
> many times. Or I could provide a parallel version where we pass in a
> PQExpBuffer that we create, one per thread, and is used by anything
> called by the parallel code. That seems like a bit of a potential
> footgun, though.

I think we should try hard to keep this localized to fmtId(), rather
than changing the callers --- the latter would be a huge readability
hit. Is there a reasonable way to have fmtId use thread-local storage
for its PQExpBuffer pointer on Windows?

regards, tom lane

Andrew Dunstan wrote:

> I have found the source of the problem I saw. dumputils.c:fmtId() uses a
> static PQExpBuffer which it initialises the first time it's called. This
> gets clobbered by simultaneous calls by Windows threads.
>
> I could just make it auto and set it up on each call, but that could
> result in a non-trivial memory leak ... it's probably called a great
> many times. Or I could provide a parallel version where we pass in a
> PQExpBuffer that we create, one per thread, and is used by anything
> called by the parallel code. That seems like a bit of a potential
> footgun, though.

Could you use a different static PQExpBuffer on each thread?
pthread_getspecific sort of thing, only to be used on Windows.

BTW does fmtQualifiedId have the same problem?

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Alvaro Herrera wrote:
> Andrew Dunstan wrote:
>
>
>> I have found the source of the problem I saw. dumputils.c:fmtId() uses a
>> static PQExpBuffer which it initialises the first time it's called. This
>> gets clobbered by simultaneous calls by Windows threads.
>>
>> I could just make it auto and set it up on each call, but that could
>> result in a non-trivial memory leak ... it's probably called a great
>> many times. Or I could provide a parallel version where we pass in a
>> PQExpBuffer that we create, one per thread, and is used by anything
>> called by the parallel code. That seems like a bit of a potential
>> footgun, though.
>>
>
> Could you use a different static PQExpBuffer on each thread?
> pthread_getspecific sort of thing, only to be used on Windows.
>

For MSVC we could declare it with "_declspec(thread)" and it would be
allocated in thread-local storage, but it looks like this isn't
supported on Mingw.

> BTW does fmtQualifiedId have the same problem?
>
>

Yes, but it's not called in threaded code - it's only in pg_dump and
only pg_restore is threaded.

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> Alvaro Herrera wrote:
>> Could you use a different static PQExpBuffer on each thread?
>> pthread_getspecific sort of thing, only to be used on Windows.

> For MSVC we could declare it with "_declspec(thread)" and it would be
> allocated in thread-local storage, but it looks like this isn't
> supported on Mingw.

Some googling suggests that thread-local storage is supported on latest
release (not clear if it's exactly the same syntax though :-().

Worst case, we could say that parallel restore isn't supported on
mingw. I'm not entirely sure why we bother with that platform at all...

regards, tom lane

Tom Lane wrote:

> Worst case, we could say that parallel restore isn't supported on
> mingw. I'm not entirely sure why we bother with that platform at all...

I think you're confusing it with cygwin ...

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Alvaro Herrera wrote:
> Tom Lane wrote:
>
>> Worst case, we could say that parallel restore isn't supported on
>> mingw. I'm not entirely sure why we bother with that platform at all...
>
> I think you're confusing it with cygwin ...

Yeah. Much as I hate working around the quirks of mingw, I definitely
think we need to keep that platform.

(yes, cygwin is another story, but let's not repeat that discussion now)

//Magnus

Andrew Dunstan wrote:
>
>
> Alvaro Herrera wrote:
>> Andrew Dunstan wrote:
>>
>>
>>> I have found the source of the problem I saw. dumputils.c:fmtId()
>>> uses a static PQExpBuffer which it initialises the first time it's
>>> called. This gets clobbered by simultaneous calls by Windows threads.
>>>
>>> I could just make it auto and set it up on each call, but that could
>>> result in a non-trivial memory leak ... it's probably called a great
>>> many times. Or I could provide a parallel version where we pass in a
>>> PQExpBuffer that we create, one per thread, and is used by anything
>>> called by the parallel code. That seems like a bit of a potential
>>> footgun, though.
>>>
>>
>> Could you use a different static PQExpBuffer on each thread?
>> pthread_getspecific sort of thing, only to be used on Windows.
>>
>
> For MSVC we could declare it with "_declspec(thread)" and it would be
> allocated in thread-local storage, but it looks like this isn't
> supported on Mingw.

Yeah, _declspec(thread) would be the easy way to do it. But you can also
use the TlsAlloc(), TlsSetValue() and friends functions directly. We do
this to use TLS in ecpg.

It requires some coding around, but for ecpg we did a macro that makes
it behave like the posix functions (see
src/interfaces/ecpg/include/ecpg-pthread-win32.h)

//Magnus

Magnus Hagander wrote:
> Andrew Dunstan wrote:
>
>> Alvaro Herrera wrote:
>>
>>> Andrew Dunstan wrote:
>>>
>>>
>>>
>>>> I have found the source of the problem I saw. dumputils.c:fmtId()
>>>> uses a static PQExpBuffer which it initialises the first time it's
>>>> called. This gets clobbered by simultaneous calls by Windows threads.
>>>>
>>>> I could just make it auto and set it up on each call, but that could
>>>> result in a non-trivial memory leak ... it's probably called a great
>>>> many times. Or I could provide a parallel version where we pass in a
>>>> PQExpBuffer that we create, one per thread, and is used by anything
>>>> called by the parallel code. That seems like a bit of a potential
>>>> footgun, though.
>>>>
>>>>
>>> Could you use a different static PQExpBuffer on each thread?
>>> pthread_getspecific sort of thing, only to be used on Windows.
>>>
>>>
>> For MSVC we could declare it with "_declspec(thread)" and it would be
>> allocated in thread-local storage, but it looks like this isn't
>> supported on Mingw.
>>
>
> Yeah, _declspec(thread) would be the easy way to do it. But you can also
> use the TlsAlloc(), TlsSetValue() and friends functions directly. We do
> this to use TLS in ecpg.
>
> It requires some coding around, but for ecpg we did a macro that makes
> it behave like the posix functions (see
> src/interfaces/ecpg/include/ecpg-pthread-win32.h)
>
>
>

Yeah, we'll just have to call TlsAlloc to set the thread handle
somewhere near program start, but that shouldn't be too intrusive.

cheers

andrew