Horton

Thread

Commits

  1. Avoid passing function pointers across process boundaries.

  2. Don't use bgw_main even to specify in-core bgworker entrypoints.

  1. logical replication launcher crash on buildfarm

    Andres Freund <andres@anarazel.de> — 2017-03-16T03:28:33Z

    Hi,

I just unstuck a bunch of my buildfarm animals.  That triggered some
spurious failures (on piculet, calliphoridae, mylodon), but also one
that doesn't really look like that:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=culicidae&dt=2017-03-16%2002%3A40%3A03

with the pertinent point being:

================== stack trace: pgsql.build/src/test/regress/tmp_check/data/core ==================
[New LWP 1894]
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
Core was generated by `postgres: bgworker: logical replication launcher                '.
Program terminated with signal SIGSEGV, Segmentation fault.
#0  0x000055e265bff5e3 in ?? ()
#0  0x000055e265bff5e3 in ?? ()
#1  0x000055d3ccabed0d in StartBackgroundWorker () at /home/andres/build/buildfarm-culicidae/HEAD/pgsql.build/../pgsql/src/backend/postmaster/bgworker.c:792
#2  0x000055d3ccacf4fc in SubPostmasterMain (argc=3, argv=0x55d3cdbb71c0) at /home/andres/build/buildfarm-culicidae/HEAD/pgsql.build/../pgsql/src/backend/postmaster/postmaster.c:4878
#3  0x000055d3cca443ea in main (argc=3, argv=0x55d3cdbb71c0) at /home/andres/build/buildfarm-culicidae/HEAD/pgsql.build/../pgsql/src/backend/main/main.c:205

it's possible that me killing things and upgrading caused this, but
given this is a backend running EXEC_BACKEND, I'm a bit suspicous that
it's more than that.  The machine is a bit backed up at the moment, so
it'll probably be a while till it's at that animal/branch again,
otherwise I'd not have mentioned this.

Greetings,

Andres Freund

  2. Re: logical replication launcher crash on buildfarm

    Andres Freund <andres@anarazel.de> — 2017-03-16T03:42:46Z

    On 2017-03-15 20:28:33 -0700, Andres Freund wrote:
> Hi,
> 
> I just unstuck a bunch of my buildfarm animals.  That triggered some
> spurious failures (on piculet, calliphoridae, mylodon), but also one
> that doesn't really look like that:
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=culicidae&dt=2017-03-16%2002%3A40%3A03
> 
> with the pertinent point being:
> 
> ================== stack trace: pgsql.build/src/test/regress/tmp_check/data/core ==================
> [New LWP 1894]
> [Thread debugging using libthread_db enabled]
> Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
> Core was generated by `postgres: bgworker: logical replication launcher                '.
> Program terminated with signal SIGSEGV, Segmentation fault.
> #0  0x000055e265bff5e3 in ?? ()
> #0  0x000055e265bff5e3 in ?? ()
> #1  0x000055d3ccabed0d in StartBackgroundWorker () at /home/andres/build/buildfarm-culicidae/HEAD/pgsql.build/../pgsql/src/backend/postmaster/bgworker.c:792
> #2  0x000055d3ccacf4fc in SubPostmasterMain (argc=3, argv=0x55d3cdbb71c0) at /home/andres/build/buildfarm-culicidae/HEAD/pgsql.build/../pgsql/src/backend/postmaster/postmaster.c:4878
> #3  0x000055d3cca443ea in main (argc=3, argv=0x55d3cdbb71c0) at /home/andres/build/buildfarm-culicidae/HEAD/pgsql.build/../pgsql/src/backend/main/main.c:205
> 
> it's possible that me killing things and upgrading caused this, but
> given this is a backend running EXEC_BACKEND, I'm a bit suspicous that
> it's more than that.  The machine is a bit backed up at the moment, so
> it'll probably be a while till it's at that animal/branch again,
> otherwise I'd not have mentioned this.

For some reason it ran again pretty soon. And I'm afraid it's indeed an
issue:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=culicidae&dt=2017-03-16%2003%3A30%3A02

- Andres

  3. Re: logical replication launcher crash on buildfarm

    Petr Jelinek <petr.jelinek@2ndquadrant.com> — 2017-03-16T08:40:48Z

    On 16/03/17 04:42, Andres Freund wrote:
> On 2017-03-15 20:28:33 -0700, Andres Freund wrote:
>> Hi,
>>
>> I just unstuck a bunch of my buildfarm animals.  That triggered some
>> spurious failures (on piculet, calliphoridae, mylodon), but also one
>> that doesn't really look like that:
>> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=culicidae&dt=2017-03-16%2002%3A40%3A03
>>
>> with the pertinent point being:
>>
>> ================== stack trace: pgsql.build/src/test/regress/tmp_check/data/core ==================
>> [New LWP 1894]
>> [Thread debugging using libthread_db enabled]
>> Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
>> Core was generated by `postgres: bgworker: logical replication launcher                '.
>> Program terminated with signal SIGSEGV, Segmentation fault.
>> #0  0x000055e265bff5e3 in ?? ()
>> #0  0x000055e265bff5e3 in ?? ()
>> #1  0x000055d3ccabed0d in StartBackgroundWorker () at /home/andres/build/buildfarm-culicidae/HEAD/pgsql.build/../pgsql/src/backend/postmaster/bgworker.c:792
>> #2  0x000055d3ccacf4fc in SubPostmasterMain (argc=3, argv=0x55d3cdbb71c0) at /home/andres/build/buildfarm-culicidae/HEAD/pgsql.build/../pgsql/src/backend/postmaster/postmaster.c:4878
>> #3  0x000055d3cca443ea in main (argc=3, argv=0x55d3cdbb71c0) at /home/andres/build/buildfarm-culicidae/HEAD/pgsql.build/../pgsql/src/backend/main/main.c:205
>>
>> it's possible that me killing things and upgrading caused this, but
>> given this is a backend running EXEC_BACKEND, I'm a bit suspicous that
>> it's more than that.  The machine is a bit backed up at the moment, so
>> it'll probably be a while till it's at that animal/branch again,
>> otherwise I'd not have mentioned this.
> 
> For some reason it ran again pretty soon. And I'm afraid it's indeed an
> issue:
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=culicidae&dt=2017-03-16%2003%3A30%3A02
> 

Hmm, I tried with EXEC_BACKEND (and with --disable-spinlocks) and it
seems to work fine on my two machines. I don't see anything else
different on culicidae though. Sadly the backtrace is not that
informative either. I'll try to investigate more but it will take time...

-- 
  Petr Jelinek                  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services

  4. Re: logical replication launcher crash on buildfarm

    Andres Freund <andres@anarazel.de> — 2017-03-16T08:44:23Z

    On 2017-03-16 09:40:48 +0100, Petr Jelinek wrote:
> On 16/03/17 04:42, Andres Freund wrote:
> > On 2017-03-15 20:28:33 -0700, Andres Freund wrote:
> >> Hi,
> >>
> >> I just unstuck a bunch of my buildfarm animals.  That triggered some
> >> spurious failures (on piculet, calliphoridae, mylodon), but also one
> >> that doesn't really look like that:
> >> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=culicidae&dt=2017-03-16%2002%3A40%3A03
> >>
> >> with the pertinent point being:
> >>
> >> ================== stack trace: pgsql.build/src/test/regress/tmp_check/data/core ==================
> >> [New LWP 1894]
> >> [Thread debugging using libthread_db enabled]
> >> Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
> >> Core was generated by `postgres: bgworker: logical replication launcher                '.
> >> Program terminated with signal SIGSEGV, Segmentation fault.
> >> #0  0x000055e265bff5e3 in ?? ()
> >> #0  0x000055e265bff5e3 in ?? ()
> >> #1  0x000055d3ccabed0d in StartBackgroundWorker () at /home/andres/build/buildfarm-culicidae/HEAD/pgsql.build/../pgsql/src/backend/postmaster/bgworker.c:792
> >> #2  0x000055d3ccacf4fc in SubPostmasterMain (argc=3, argv=0x55d3cdbb71c0) at /home/andres/build/buildfarm-culicidae/HEAD/pgsql.build/../pgsql/src/backend/postmaster/postmaster.c:4878
> >> #3  0x000055d3cca443ea in main (argc=3, argv=0x55d3cdbb71c0) at /home/andres/build/buildfarm-culicidae/HEAD/pgsql.build/../pgsql/src/backend/main/main.c:205
> >>
> >> it's possible that me killing things and upgrading caused this, but
> >> given this is a backend running EXEC_BACKEND, I'm a bit suspicous that
> >> it's more than that.  The machine is a bit backed up at the moment, so
> >> it'll probably be a while till it's at that animal/branch again,
> >> otherwise I'd not have mentioned this.
> > 
> > For some reason it ran again pretty soon. And I'm afraid it's indeed an
> > issue:
> > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=culicidae&dt=2017-03-16%2003%3A30%3A02
> > 
> 
> Hmm, I tried with EXEC_BACKEND (and with --disable-spinlocks) and it
> seems to work fine on my two machines. I don't see anything else
> different on culicidae though. Sadly the backtrace is not that
> informative either. I'll try to investigate more but it will take time...

I can give you a login to that machine, it doesn't do anything but run
buildfarm animals...  Will have to be my tomorrow however.

(Also need to fix config for older branches that don't work with
the upgraded ssl. This is a really bad situation :()

Greetings,

Andres Freund

  5. Re: logical replication launcher crash on buildfarm

    Petr Jelinek <petr.jelinek@2ndquadrant.com> — 2017-03-16T08:51:42Z

    On 16/03/17 09:44, Andres Freund wrote:
> On 2017-03-16 09:40:48 +0100, Petr Jelinek wrote:
>> On 16/03/17 04:42, Andres Freund wrote:
>>> On 2017-03-15 20:28:33 -0700, Andres Freund wrote:
>>>> Hi,
>>>>
>>>> I just unstuck a bunch of my buildfarm animals.  That triggered some
>>>> spurious failures (on piculet, calliphoridae, mylodon), but also one
>>>> that doesn't really look like that:
>>>> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=culicidae&dt=2017-03-16%2002%3A40%3A03
>>>>
>>>> with the pertinent point being:
>>>>
>>>> ================== stack trace: pgsql.build/src/test/regress/tmp_check/data/core ==================
>>>> [New LWP 1894]
>>>> [Thread debugging using libthread_db enabled]
>>>> Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
>>>> Core was generated by `postgres: bgworker: logical replication launcher                '.
>>>> Program terminated with signal SIGSEGV, Segmentation fault.
>>>> #0  0x000055e265bff5e3 in ?? ()
>>>> #0  0x000055e265bff5e3 in ?? ()
>>>> #1  0x000055d3ccabed0d in StartBackgroundWorker () at /home/andres/build/buildfarm-culicidae/HEAD/pgsql.build/../pgsql/src/backend/postmaster/bgworker.c:792
>>>> #2  0x000055d3ccacf4fc in SubPostmasterMain (argc=3, argv=0x55d3cdbb71c0) at /home/andres/build/buildfarm-culicidae/HEAD/pgsql.build/../pgsql/src/backend/postmaster/postmaster.c:4878
>>>> #3  0x000055d3cca443ea in main (argc=3, argv=0x55d3cdbb71c0) at /home/andres/build/buildfarm-culicidae/HEAD/pgsql.build/../pgsql/src/backend/main/main.c:205
>>>>
>>>> it's possible that me killing things and upgrading caused this, but
>>>> given this is a backend running EXEC_BACKEND, I'm a bit suspicous that
>>>> it's more than that.  The machine is a bit backed up at the moment, so
>>>> it'll probably be a while till it's at that animal/branch again,
>>>> otherwise I'd not have mentioned this.
>>>
>>> For some reason it ran again pretty soon. And I'm afraid it's indeed an
>>> issue:
>>> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=culicidae&dt=2017-03-16%2003%3A30%3A02
>>>
>>
>> Hmm, I tried with EXEC_BACKEND (and with --disable-spinlocks) and it
>> seems to work fine on my two machines. I don't see anything else
>> different on culicidae though. Sadly the backtrace is not that
>> informative either. I'll try to investigate more but it will take time...
> 
> I can give you a login to that machine, it doesn't do anything but run
> buildfarm animals...  Will have to be my tomorrow however.
> 

That would be helpful, thanks.

-- 
  Petr Jelinek                  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services

  6. Re: logical replication launcher crash on buildfarm

    Andres Freund <andres@anarazel.de> — 2017-03-16T08:53:22Z

    On 2017-03-16 09:40:48 +0100, Petr Jelinek wrote:
> On 16/03/17 04:42, Andres Freund wrote:
> > On 2017-03-15 20:28:33 -0700, Andres Freund wrote:
> >> Hi,
> >>
> >> I just unstuck a bunch of my buildfarm animals.  That triggered some
> >> spurious failures (on piculet, calliphoridae, mylodon), but also one
> >> that doesn't really look like that:
> >> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=culicidae&dt=2017-03-16%2002%3A40%3A03
> >>
> >> with the pertinent point being:
> >>
> >> ================== stack trace: pgsql.build/src/test/regress/tmp_check/data/core ==================
> >> [New LWP 1894]
> >> [Thread debugging using libthread_db enabled]
> >> Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
> >> Core was generated by `postgres: bgworker: logical replication launcher                '.
> >> Program terminated with signal SIGSEGV, Segmentation fault.
> >> #0  0x000055e265bff5e3 in ?? ()
> >> #0  0x000055e265bff5e3 in ?? ()
> >> #1  0x000055d3ccabed0d in StartBackgroundWorker () at /home/andres/build/buildfarm-culicidae/HEAD/pgsql.build/../pgsql/src/backend/postmaster/bgworker.c:792
> >> #2  0x000055d3ccacf4fc in SubPostmasterMain (argc=3, argv=0x55d3cdbb71c0) at /home/andres/build/buildfarm-culicidae/HEAD/pgsql.build/../pgsql/src/backend/postmaster/postmaster.c:4878
> >> #3  0x000055d3cca443ea in main (argc=3, argv=0x55d3cdbb71c0) at /home/andres/build/buildfarm-culicidae/HEAD/pgsql.build/../pgsql/src/backend/main/main.c:205
> >>
> >> it's possible that me killing things and upgrading caused this, but
> >> given this is a backend running EXEC_BACKEND, I'm a bit suspicous that
> >> it's more than that.  The machine is a bit backed up at the moment, so
> >> it'll probably be a while till it's at that animal/branch again,
> >> otherwise I'd not have mentioned this.
> > 
> > For some reason it ran again pretty soon. And I'm afraid it's indeed an
> > issue:
> > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=culicidae&dt=2017-03-16%2003%3A30%3A02
> > 
> 
> Hmm, I tried with EXEC_BACKEND (and with --disable-spinlocks) and it
> seems to work fine on my two machines. I don't see anything else
> different on culicidae though. Sadly the backtrace is not that
> informative either. I'll try to investigate more but it will take time...

Worthwhile additional failure:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=culicidae&dt=2017-03-16%2002%3A55%3A01

Same animal, also EXEC_BACKEND, but 9.6.

A quick look at the relevant line:
	/*
	 * If bgw_main is set, we use that value as the initial entrypoint.
	 * However, if the library containing the entrypoint wasn't loaded at
	 * postmaster startup time, passing it as a direct function pointer is not
	 * possible.  To work around that, we allow callers for whom a function
	 * pointer is not available to pass a library name (which will be loaded,
	 * if necessary) and a function name (which will be looked up in the named
	 * library).
	 */
	if (worker->bgw_main != NULL)
		entrypt = worker->bgw_main;

makes the issue clear - we appear to be assuming that bgw_main is
meaningful across processes.  Which it isn't in the EXEC_BACKEND case
when ASLR is in use...

This kinda sounds familiar, but a quick google search doesn't find
anything relevant.

Greetings,

Andres Freund

  7. Re: logical replication launcher crash on buildfarm

    Petr Jelinek <petr.jelinek@2ndquadrant.com> — 2017-03-16T09:13:37Z

    On 16/03/17 09:53, Andres Freund wrote:
> On 2017-03-16 09:40:48 +0100, Petr Jelinek wrote:
>> On 16/03/17 04:42, Andres Freund wrote:
>>> On 2017-03-15 20:28:33 -0700, Andres Freund wrote:
>>>> Hi,
>>>>
>>>> I just unstuck a bunch of my buildfarm animals.  That triggered some
>>>> spurious failures (on piculet, calliphoridae, mylodon), but also one
>>>> that doesn't really look like that:
>>>> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=culicidae&dt=2017-03-16%2002%3A40%3A03
>>>>
>>>> with the pertinent point being:
>>>>
>>>> ================== stack trace: pgsql.build/src/test/regress/tmp_check/data/core ==================
>>>> [New LWP 1894]
>>>> [Thread debugging using libthread_db enabled]
>>>> Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
>>>> Core was generated by `postgres: bgworker: logical replication launcher                '.
>>>> Program terminated with signal SIGSEGV, Segmentation fault.
>>>> #0  0x000055e265bff5e3 in ?? ()
>>>> #0  0x000055e265bff5e3 in ?? ()
>>>> #1  0x000055d3ccabed0d in StartBackgroundWorker () at /home/andres/build/buildfarm-culicidae/HEAD/pgsql.build/../pgsql/src/backend/postmaster/bgworker.c:792
>>>> #2  0x000055d3ccacf4fc in SubPostmasterMain (argc=3, argv=0x55d3cdbb71c0) at /home/andres/build/buildfarm-culicidae/HEAD/pgsql.build/../pgsql/src/backend/postmaster/postmaster.c:4878
>>>> #3  0x000055d3cca443ea in main (argc=3, argv=0x55d3cdbb71c0) at /home/andres/build/buildfarm-culicidae/HEAD/pgsql.build/../pgsql/src/backend/main/main.c:205
>>>>
>>>> it's possible that me killing things and upgrading caused this, but
>>>> given this is a backend running EXEC_BACKEND, I'm a bit suspicous that
>>>> it's more than that.  The machine is a bit backed up at the moment, so
>>>> it'll probably be a while till it's at that animal/branch again,
>>>> otherwise I'd not have mentioned this.
>>>
>>> For some reason it ran again pretty soon. And I'm afraid it's indeed an
>>> issue:
>>> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=culicidae&dt=2017-03-16%2003%3A30%3A02
>>>
>>
>> Hmm, I tried with EXEC_BACKEND (and with --disable-spinlocks) and it
>> seems to work fine on my two machines. I don't see anything else
>> different on culicidae though. Sadly the backtrace is not that
>> informative either. I'll try to investigate more but it will take time...
> 
> Worthwhile additional failure:
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=culicidae&dt=2017-03-16%2002%3A55%3A01
> 
> Same animal, also EXEC_BACKEND, but 9.6.
> 
> A quick look at the relevant line:
> 	/*
> 	 * If bgw_main is set, we use that value as the initial entrypoint.
> 	 * However, if the library containing the entrypoint wasn't loaded at
> 	 * postmaster startup time, passing it as a direct function pointer is not
> 	 * possible.  To work around that, we allow callers for whom a function
> 	 * pointer is not available to pass a library name (which will be loaded,
> 	 * if necessary) and a function name (which will be looked up in the named
> 	 * library).
> 	 */
> 	if (worker->bgw_main != NULL)
> 		entrypt = worker->bgw_main;
> 
> makes the issue clear - we appear to be assuming that bgw_main is
> meaningful across processes.  Which it isn't in the EXEC_BACKEND case
> when ASLR is in use...
> 
> This kinda sounds familiar, but a quick google search doesn't find
> anything relevant.

Hmm now that you mention it, I remember discussing something similar
with you last year in Dallas in regards to parallel query. IIRC Windows
should not have this problem but other systems with EXEC_BACKEND do.
Don't remember the details though.

-- 
  Petr Jelinek                  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services

  8. Re: logical replication launcher crash on buildfarm

    Robert Haas <robertmhaas@gmail.com> — 2017-03-16T13:27:59Z

    On Thu, Mar 16, 2017 at 5:13 AM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:
> Hmm now that you mention it, I remember discussing something similar
> with you last year in Dallas in regards to parallel query. IIRC Windows
> should not have this problem but other systems with EXEC_BACKEND do.
> Don't remember the details though.

Generally, extension code can't use bgw_main safely, and must use
bgw_library_name and bgw_function_name.  But bgw_main is supposedly
safe for core code.  If it's not even safe there, then I guess we
should remove it entirely as a useless foot-gun.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  9. Re: logical replication launcher crash on buildfarm

    Andres Freund <andres@anarazel.de> — 2017-03-16T18:55:05Z

    On 2017-03-16 09:27:59 -0400, Robert Haas wrote:
> On Thu, Mar 16, 2017 at 5:13 AM, Petr Jelinek
> <petr.jelinek@2ndquadrant.com> wrote:
> > Hmm now that you mention it, I remember discussing something similar
> > with you last year in Dallas in regards to parallel query. IIRC Windows
> > should not have this problem but other systems with EXEC_BACKEND do.
> > Don't remember the details though.
> 
> Generally, extension code can't use bgw_main safely, and must use
> bgw_library_name and bgw_function_name.  But bgw_main is supposedly
> safe for core code.

I indeed think it's not safe, and it's going to get less and less safe
on windows (or EXEC_BACKEND).  I don't think we can afford to disable
ASLR in the long run (I indeed supect that'll just be disallowed at some
point), and that's the only thing making it safe-ish in combination with
EXEC_BACKEND.


> If it's not even safe there, then I guess we should remove it entirely
> as a useless foot-gun.

I indeed think that's the right consequence.  One question is what to
replace it with exactly - are we guaranteed we can dynamically lookup
symbols by name in the main binary on every platform?  Alternatively we
can just hardcode a bunch of bgw_function_name values that are matched
to specific functions if bgw_library_name is NULL - I suspect that'd be
the easiest / least worrysome portability-wise.

Greetings,

Andres Freund

  10. Re: logical replication launcher crash on buildfarm

    Robert Haas <robertmhaas@gmail.com> — 2017-03-16T19:12:20Z

    On Thu, Mar 16, 2017 at 2:55 PM, Andres Freund <andres@anarazel.de> wrote:
> I indeed think it's not safe, and it's going to get less and less safe
> on windows (or EXEC_BACKEND).  I don't think we can afford to disable
> ASLR in the long run (I indeed supect that'll just be disallowed at some
> point), and that's the only thing making it safe-ish in combination with
> EXEC_BACKEND.

Ugh.

>> If it's not even safe there, then I guess we should remove it entirely
>> as a useless foot-gun.
>
> I indeed think that's the right consequence.  One question is what to
> replace it with exactly - are we guaranteed we can dynamically lookup
> symbols by name in the main binary on every platform?

I don't know the answer to that question.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  11. Re: logical replication launcher crash on buildfarm

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2017-03-17T03:24:07Z

    On 3/16/17 14:55, Andres Freund wrote:
> I indeed think that's the right consequence.  One question is what to
> replace it with exactly - are we guaranteed we can dynamically lookup
> symbols by name in the main binary on every platform?

I think there is probably a way to do this on all platforms.  But it
seems that at least the Windows port of pg_dlopen would need to be
updated to support this.

> Alternatively we
> can just hardcode a bunch of bgw_function_name values that are matched
> to specific functions if bgw_library_name is NULL - I suspect that'd be
> the easiest / least worrysome portability-wise.

Basically a variant of fmgrtab, which addresses the same sort of problem.


-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  12. Re: logical replication launcher crash on buildfarm

    Andres Freund <andres@anarazel.de> — 2017-03-27T16:50:23Z

    On 2017-03-16 10:13:37 +0100, Petr Jelinek wrote:
> On 16/03/17 09:53, Andres Freund wrote:
> > On 2017-03-16 09:40:48 +0100, Petr Jelinek wrote:
> >> On 16/03/17 04:42, Andres Freund wrote:
> >>> On 2017-03-15 20:28:33 -0700, Andres Freund wrote:
> >>>> Hi,
> >>>>
> >>>> I just unstuck a bunch of my buildfarm animals.  That triggered some
> >>>> spurious failures (on piculet, calliphoridae, mylodon), but also one
> >>>> that doesn't really look like that:
> >>>> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=culicidae&dt=2017-03-16%2002%3A40%3A03
> >>>>
> >>>> with the pertinent point being:
> >>>>
> >>>> ================== stack trace: pgsql.build/src/test/regress/tmp_check/data/core ==================
> >>>> [New LWP 1894]
> >>>> [Thread debugging using libthread_db enabled]
> >>>> Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
> >>>> Core was generated by `postgres: bgworker: logical replication launcher                '.
> >>>> Program terminated with signal SIGSEGV, Segmentation fault.
> >>>> #0  0x000055e265bff5e3 in ?? ()
> >>>> #0  0x000055e265bff5e3 in ?? ()
> >>>> #1  0x000055d3ccabed0d in StartBackgroundWorker () at /home/andres/build/buildfarm-culicidae/HEAD/pgsql.build/../pgsql/src/backend/postmaster/bgworker.c:792
> >>>> #2  0x000055d3ccacf4fc in SubPostmasterMain (argc=3, argv=0x55d3cdbb71c0) at /home/andres/build/buildfarm-culicidae/HEAD/pgsql.build/../pgsql/src/backend/postmaster/postmaster.c:4878
> >>>> #3  0x000055d3cca443ea in main (argc=3, argv=0x55d3cdbb71c0) at /home/andres/build/buildfarm-culicidae/HEAD/pgsql.build/../pgsql/src/backend/main/main.c:205
> >>>>
> >>>> it's possible that me killing things and upgrading caused this, but
> >>>> given this is a backend running EXEC_BACKEND, I'm a bit suspicous that
> >>>> it's more than that.  The machine is a bit backed up at the moment, so
> >>>> it'll probably be a while till it's at that animal/branch again,
> >>>> otherwise I'd not have mentioned this.
> >>>
> >>> For some reason it ran again pretty soon. And I'm afraid it's indeed an
> >>> issue:
> >>> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=culicidae&dt=2017-03-16%2003%3A30%3A02
> >>>
> >>
> >> Hmm, I tried with EXEC_BACKEND (and with --disable-spinlocks) and it
> >> seems to work fine on my two machines. I don't see anything else
> >> different on culicidae though. Sadly the backtrace is not that
> >> informative either. I'll try to investigate more but it will take time...
> > 
> > Worthwhile additional failure:
> > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=culicidae&dt=2017-03-16%2002%3A55%3A01
> > 
> > Same animal, also EXEC_BACKEND, but 9.6.
> > 
> > A quick look at the relevant line:
> > 	/*
> > 	 * If bgw_main is set, we use that value as the initial entrypoint.
> > 	 * However, if the library containing the entrypoint wasn't loaded at
> > 	 * postmaster startup time, passing it as a direct function pointer is not
> > 	 * possible.  To work around that, we allow callers for whom a function
> > 	 * pointer is not available to pass a library name (which will be loaded,
> > 	 * if necessary) and a function name (which will be looked up in the named
> > 	 * library).
> > 	 */
> > 	if (worker->bgw_main != NULL)
> > 		entrypt = worker->bgw_main;
> > 
> > makes the issue clear - we appear to be assuming that bgw_main is
> > meaningful across processes.  Which it isn't in the EXEC_BACKEND case
> > when ASLR is in use...
> > 
> > This kinda sounds familiar, but a quick google search doesn't find
> > anything relevant.

Robert, Petr, either of you planning to fix this (as outlined elsewhere
in the thred)?


> Hmm now that you mention it, I remember discussing something similar
> with you last year in Dallas in regards to parallel query. IIRC Windows
> should not have this problem but other systems with EXEC_BACKEND do.
> Don't remember the details though.

Don't think that's reliable, only works as long as the binary is
compiled without position independent code.

Greetings,

Andres Freund

  13. Re: logical replication launcher crash on buildfarm

    Robert Haas <robertmhaas@gmail.com> — 2017-03-27T17:01:11Z

    On Mon, Mar 27, 2017 at 12:50 PM, Andres Freund <andres@anarazel.de> wrote:
> Robert, Petr, either of you planning to fix this (as outlined elsewhere
> in the thred)?

Oh, I didn't realize anybody was looking to me to fix this.  I sort of
thought that it was fallout from the logical replication patch and
that Petr or Peter would deal with it.  If that's not the case, I'm
not totally unwilling to take a whack at it, but I don't have much
personal enthusiasm for trying to figure out how to make dynamic
loading on the postgres binary itself work everywhere, so if it falls
to me to fix, it's likely to get a hard-coded check for some
hard-coded name.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  14. Re: logical replication launcher crash on buildfarm

    Andres Freund <andres@anarazel.de> — 2017-03-27T17:05:24Z

    On 2017-03-27 13:01:11 -0400, Robert Haas wrote:
> On Mon, Mar 27, 2017 at 12:50 PM, Andres Freund <andres@anarazel.de> wrote:
> > Robert, Petr, either of you planning to fix this (as outlined elsewhere
> > in the thred)?
> 
> Oh, I didn't realize anybody was looking to me to fix this.

Well, it's borked in 9.6.  I'm starting to get annoyed by culicidae's
failures ;)


> I sort of thought that it was fallout from the logical replication
> patch and that Petr or Peter would deal with it.  If that's not the
> case, I'm not totally unwilling to take a whack at it, but I don't
> have much personal enthusiasm for trying to figure out how to make
> dynamic loading on the postgres binary itself work everywhere, so if
> it falls to me to fix, it's likely to get a hard-coded check for some
> hard-coded name.

I'm all for that approach - there seems very little upside in the
dynamic loading approach.  Just defining a bgw_entry_points[enum
BuiltinBGWorkerType] -> bgworker_main_type array seems to be simple
enough - it's not like we're going to add new types of builtin bgworkers
at runtime.

- Andres

  15. Re: logical replication launcher crash on buildfarm

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-03-27T17:47:02Z

    Robert Haas <robertmhaas@gmail.com> writes:
> ... I don't have much
> personal enthusiasm for trying to figure out how to make dynamic
> loading on the postgres binary itself work everywhere, so if it falls
> to me to fix, it's likely to get a hard-coded check for some
> hard-coded name.

+1.  This seems like no time to be buying into brand new portability
requirements without a very pressing need to do so; and this patch
doesn't appear to create one.

			regards, tom lane

  16. Re: logical replication launcher crash on buildfarm

    Petr Jelinek <petr.jelinek@2ndquadrant.com> — 2017-03-28T01:31:02Z

    On 27/03/17 19:01, Robert Haas wrote:
> On Mon, Mar 27, 2017 at 12:50 PM, Andres Freund <andres@anarazel.de> wrote:
>> Robert, Petr, either of you planning to fix this (as outlined elsewhere
>> in the thred)?
> 
> Oh, I didn't realize anybody was looking to me to fix this.  I sort of
> thought that it was fallout from the logical replication patch and
> that Petr or Peter would deal with it.  If that's not the case, I'm
> not totally unwilling to take a whack at it, but I don't have much
> personal enthusiasm for trying to figure out how to make dynamic
> loading on the postgres binary itself work everywhere, so if it falls
> to me to fix, it's likely to get a hard-coded check for some
> hard-coded name.
> 

It affects parallel workers same way, I'll write patch for HEAD soon,
9.6 probably with some delay. I'll expand the InternalBgWorkers thing
that was added with logical replication to handle this in similar
fashion how we do fmgrtab.

-- 
  Petr Jelinek                  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services

  17. Re: logical replication launcher crash on buildfarm

    Petr Jelinek <petr.jelinek@2ndquadrant.com> — 2017-03-28T01:47:50Z

    On 28/03/17 03:31, Petr Jelinek wrote:
> On 27/03/17 19:01, Robert Haas wrote:
>> On Mon, Mar 27, 2017 at 12:50 PM, Andres Freund <andres@anarazel.de> wrote:
>>> Robert, Petr, either of you planning to fix this (as outlined elsewhere
>>> in the thred)?
>>
>> Oh, I didn't realize anybody was looking to me to fix this.  I sort of
>> thought that it was fallout from the logical replication patch and
>> that Petr or Peter would deal with it.  If that's not the case, I'm
>> not totally unwilling to take a whack at it, but I don't have much
>> personal enthusiasm for trying to figure out how to make dynamic
>> loading on the postgres binary itself work everywhere, so if it falls
>> to me to fix, it's likely to get a hard-coded check for some
>> hard-coded name.
>>
> 
> It affects parallel workers same way, I'll write patch for HEAD soon,
> 9.6 probably with some delay. I'll expand the InternalBgWorkers thing
> that was added with logical replication to handle this in similar
> fashion how we do fmgrtab.
> 

Btw now that I look at the code, I guess we'll want to get rid of
bgw_main completely in HEAD given that we can't guarantee it will be
valid even for shared_preload_library libraries. For older branches I
would leave things as they are in this regard as there don't seem to be
any immediate issue for standard binaries.

-- 
  Petr Jelinek                  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services

  18. Re: logical replication launcher crash on buildfarm

    Andres Freund <andres@anarazel.de> — 2017-03-28T02:04:22Z

    On 2017-03-28 03:47:50 +0200, Petr Jelinek wrote:
> On 28/03/17 03:31, Petr Jelinek wrote:
> > On 27/03/17 19:01, Robert Haas wrote:
> >> On Mon, Mar 27, 2017 at 12:50 PM, Andres Freund <andres@anarazel.de> wrote:
> >>> Robert, Petr, either of you planning to fix this (as outlined elsewhere
> >>> in the thred)?
> >>
> >> Oh, I didn't realize anybody was looking to me to fix this.  I sort of
> >> thought that it was fallout from the logical replication patch and
> >> that Petr or Peter would deal with it.  If that's not the case, I'm
> >> not totally unwilling to take a whack at it, but I don't have much
> >> personal enthusiasm for trying to figure out how to make dynamic
> >> loading on the postgres binary itself work everywhere, so if it falls
> >> to me to fix, it's likely to get a hard-coded check for some
> >> hard-coded name.
> >>
> > 
> > It affects parallel workers same way, I'll write patch for HEAD soon,
> > 9.6 probably with some delay. I'll expand the InternalBgWorkers thing
> > that was added with logical replication to handle this in similar
> > fashion how we do fmgrtab.
> > 
> 
> Btw now that I look at the code, I guess we'll want to get rid of
> bgw_main completely in HEAD given that we can't guarantee it will be
> valid even for shared_preload_library libraries. For older branches I
> would leave things as they are in this regard as there don't seem to be
> any immediate issue for standard binaries.

As long as you fix it so culicidae is happy (in 9.6) ;).  I think it's
fine to just introduce bgw_builtin_id or such, and leave the bgw_main
code in place in < HEAD.


Greetings,

Andres Freund

  19. Re: logical replication launcher crash on buildfarm

    Robert Haas <robertmhaas@gmail.com> — 2017-03-28T02:46:00Z

    On Mon, Mar 27, 2017 at 10:04 PM, Andres Freund <andres@anarazel.de> wrote:
>> Btw now that I look at the code, I guess we'll want to get rid of
>> bgw_main completely in HEAD given that we can't guarantee it will be
>> valid even for shared_preload_library libraries. For older branches I
>> would leave things as they are in this regard as there don't seem to be
>> any immediate issue for standard binaries.
>
> As long as you fix it so culicidae is happy (in 9.6) ;).  I think it's
> fine to just introduce bgw_builtin_id or such, and leave the bgw_main
> code in place in < HEAD.

I wasn't thinking of introducing bgw_builtin_id.  My idea was just
along the lines of

if (bgw_library_name == NULL && bgw_function_name != NULL)
{
    if (strcmp(bgw_function_name, "ParallelQueryMain") == 0)
       ParallelQueryMain(blah);
    else if (strcmp(bgw_function_name, "LogicalReplicationMain") == 0)
       LogicalReplicationMain(blah);
}

I think something like that is certainly better for the back-branches,
because it doesn't cause an ABI break.  But I think it would also be
fine for master.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  20. Re: logical replication launcher crash on buildfarm

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-03-28T02:51:53Z

    Robert Haas <robertmhaas@gmail.com> writes:
> I wasn't thinking of introducing bgw_builtin_id.  My idea was just
> along the lines of

> if (bgw_library_name == NULL && bgw_function_name != NULL)
> {
>     if (strcmp(bgw_function_name, "ParallelQueryMain") == 0)
>        ParallelQueryMain(blah);
>     else if (strcmp(bgw_function_name, "LogicalReplicationMain") == 0)
>        LogicalReplicationMain(blah);
> }

> I think something like that is certainly better for the back-branches,
> because it doesn't cause an ABI break.  But I think it would also be
> fine for master.

That seems perfectly reasonable from here: surely the cost of a couple
of strcmp's is trivial in comparison to a process launch.

We can redesign the API whenever this way starts getting unwieldy,
but that's likely to be quite some time away.

			regards, tom lane

  21. Re: logical replication launcher crash on buildfarm

    Petr Jelinek <petr.jelinek@2ndquadrant.com> — 2017-03-28T03:20:44Z

    On 28/03/17 04:46, Robert Haas wrote:
> On Mon, Mar 27, 2017 at 10:04 PM, Andres Freund <andres@anarazel.de> wrote:
>>> Btw now that I look at the code, I guess we'll want to get rid of
>>> bgw_main completely in HEAD given that we can't guarantee it will be
>>> valid even for shared_preload_library libraries. For older branches I
>>> would leave things as they are in this regard as there don't seem to be
>>> any immediate issue for standard binaries.
>>
>> As long as you fix it so culicidae is happy (in 9.6) ;).  I think it's
>> fine to just introduce bgw_builtin_id or such, and leave the bgw_main
>> code in place in < HEAD.
> 
> I wasn't thinking of introducing bgw_builtin_id.  My idea was just
> along the lines of
> 
> if (bgw_library_name == NULL && bgw_function_name != NULL)
> {
>     if (strcmp(bgw_function_name, "ParallelQueryMain") == 0)
>        ParallelQueryMain(blah);
>     else if (strcmp(bgw_function_name, "LogicalReplicationMain") == 0)
>        LogicalReplicationMain(blah);
> }
> 
> I think something like that is certainly better for the back-branches,
> because it doesn't cause an ABI break.  But I think it would also be
> fine for master.
> 

I had something slightly more complex like the attached in mind.

-- 
  Petr Jelinek                  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services

  22. Re: logical replication launcher crash on buildfarm

    Robert Haas <robertmhaas@gmail.com> — 2017-03-28T15:55:39Z

    On Mon, Mar 27, 2017 at 11:20 PM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:
> On 28/03/17 04:46, Robert Haas wrote:
>> On Mon, Mar 27, 2017 at 10:04 PM, Andres Freund <andres@anarazel.de> wrote:
>>>> Btw now that I look at the code, I guess we'll want to get rid of
>>>> bgw_main completely in HEAD given that we can't guarantee it will be
>>>> valid even for shared_preload_library libraries. For older branches I
>>>> would leave things as they are in this regard as there don't seem to be
>>>> any immediate issue for standard binaries.
>>>
>>> As long as you fix it so culicidae is happy (in 9.6) ;).  I think it's
>>> fine to just introduce bgw_builtin_id or such, and leave the bgw_main
>>> code in place in < HEAD.
>>
>> I wasn't thinking of introducing bgw_builtin_id.  My idea was just
>> along the lines of
>>
>> if (bgw_library_name == NULL && bgw_function_name != NULL)
>> {
>>     if (strcmp(bgw_function_name, "ParallelQueryMain") == 0)
>>        ParallelQueryMain(blah);
>>     else if (strcmp(bgw_function_name, "LogicalReplicationMain") == 0)
>>        LogicalReplicationMain(blah);
>> }
>>
>> I think something like that is certainly better for the back-branches,
>> because it doesn't cause an ABI break.  But I think it would also be
>> fine for master.
>>
>
> I had something slightly more complex like the attached in mind.

Seems broadly reasonable on a quick look, but I think we should leave
bgw_main intact in 9.6.  It may be working for fine for people who
don't care about Windows, and I'd rather not break it gratuitously.
Can we have two patches, one of which converts the internal stuff to
use the new mechanism and a second of which removes bgw_main?  The
second one, at least, also needs to update the documentation.  (A good
practice when removing an identifier is to run 'git grep
thing_i_am_removing' after removing it...)

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  23. Re: logical replication launcher crash on buildfarm

    Petr Jelinek <petr.jelinek@2ndquadrant.com> — 2017-03-28T16:05:35Z

    On 28/03/17 17:55, Robert Haas wrote:
> On Mon, Mar 27, 2017 at 11:20 PM, Petr Jelinek
> <petr.jelinek@2ndquadrant.com> wrote:
>> On 28/03/17 04:46, Robert Haas wrote:
>>> On Mon, Mar 27, 2017 at 10:04 PM, Andres Freund <andres@anarazel.de> wrote:
>>>>> Btw now that I look at the code, I guess we'll want to get rid of
>>>>> bgw_main completely in HEAD given that we can't guarantee it will be
>>>>> valid even for shared_preload_library libraries. For older branches I
>>>>> would leave things as they are in this regard as there don't seem to be
>>>>> any immediate issue for standard binaries.
>>>>
>>>> As long as you fix it so culicidae is happy (in 9.6) ;).  I think it's
>>>> fine to just introduce bgw_builtin_id or such, and leave the bgw_main
>>>> code in place in < HEAD.
>>>
>>> I wasn't thinking of introducing bgw_builtin_id.  My idea was just
>>> along the lines of
>>>
>>> if (bgw_library_name == NULL && bgw_function_name != NULL)
>>> {
>>>     if (strcmp(bgw_function_name, "ParallelQueryMain") == 0)
>>>        ParallelQueryMain(blah);
>>>     else if (strcmp(bgw_function_name, "LogicalReplicationMain") == 0)
>>>        LogicalReplicationMain(blah);
>>> }
>>>
>>> I think something like that is certainly better for the back-branches,
>>> because it doesn't cause an ABI break.  But I think it would also be
>>> fine for master.
>>>
>>
>> I had something slightly more complex like the attached in mind.
> 
> Seems broadly reasonable on a quick look, but I think we should leave
> bgw_main intact in 9.6.  It may be working for fine for people who
> don't care about Windows, and I'd rather not break it gratuitously.
> Can we have two patches, one of which converts the internal stuff to
> use the new mechanism and a second of which removes bgw_main?  The
> second one, at least, also needs to update the documentation.  (A good
> practice when removing an identifier is to run 'git grep
> thing_i_am_removing' after removing it...)
> 

Yes I agree (and I said it in the thread) I plan to submit the 9.6 too,
I just sent what I had before having it all ready mainly because you
started talking about code already and I didn't want you to waste time
with it needlessly, but it was already 5AM for me ;).

-- 
  Petr Jelinek                  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services

  24. Re: logical replication launcher crash on buildfarm

    Petr Jelinek <petr.jelinek@2ndquadrant.com> — 2017-03-28T23:29:48Z

    On 28/03/17 18:05, Petr Jelinek wrote:
> On 28/03/17 17:55, Robert Haas wrote:
>> On Mon, Mar 27, 2017 at 11:20 PM, Petr Jelinek
>> <petr.jelinek@2ndquadrant.com> wrote:
>>> On 28/03/17 04:46, Robert Haas wrote:
>>>> On Mon, Mar 27, 2017 at 10:04 PM, Andres Freund <andres@anarazel.de> wrote:
>>>>>> Btw now that I look at the code, I guess we'll want to get rid of
>>>>>> bgw_main completely in HEAD given that we can't guarantee it will be
>>>>>> valid even for shared_preload_library libraries. For older branches I
>>>>>> would leave things as they are in this regard as there don't seem to be
>>>>>> any immediate issue for standard binaries.
>>>>>
>>>>> As long as you fix it so culicidae is happy (in 9.6) ;).  I think it's
>>>>> fine to just introduce bgw_builtin_id or such, and leave the bgw_main
>>>>> code in place in < HEAD.
>>>>
>>>> I wasn't thinking of introducing bgw_builtin_id.  My idea was just
>>>> along the lines of
>>>>
>>>> if (bgw_library_name == NULL && bgw_function_name != NULL)
>>>> {
>>>>     if (strcmp(bgw_function_name, "ParallelQueryMain") == 0)
>>>>        ParallelQueryMain(blah);
>>>>     else if (strcmp(bgw_function_name, "LogicalReplicationMain") == 0)
>>>>        LogicalReplicationMain(blah);
>>>> }
>>>>
>>>> I think something like that is certainly better for the back-branches,
>>>> because it doesn't cause an ABI break.  But I think it would also be
>>>> fine for master.
>>>>
>>>
>>> I had something slightly more complex like the attached in mind.
>>
>> Seems broadly reasonable on a quick look, but I think we should leave
>> bgw_main intact in 9.6.  It may be working for fine for people who
>> don't care about Windows, and I'd rather not break it gratuitously.
>> Can we have two patches, one of which converts the internal stuff to
>> use the new mechanism and a second of which removes bgw_main?  The
>> second one, at least, also needs to update the documentation.  (A good
>> practice when removing an identifier is to run 'git grep
>> thing_i_am_removing' after removing it...)
>>
> 
> Yes I agree (and I said it in the thread) I plan to submit the 9.6 too,
> I just sent what I had before having it all ready mainly because you
> started talking about code already and I didn't want you to waste time
> with it needlessly, but it was already 5AM for me ;).
> 

Okay finally got to it, the 9.6 adds similar mechanism as HEAD (it could
be simpler there but it seems like same code is better) but does not
remove the bgw_main.

-- 
  Petr Jelinek                  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services

  25. Re: logical replication launcher crash on buildfarm

    Petr Jelinek <petr.jelinek@2ndquadrant.com> — 2017-03-28T23:39:27Z

    On 29/03/17 01:29, Petr Jelinek wrote:
> On 28/03/17 18:05, Petr Jelinek wrote:
>> On 28/03/17 17:55, Robert Haas wrote:
>>> On Mon, Mar 27, 2017 at 11:20 PM, Petr Jelinek
>>> <petr.jelinek@2ndquadrant.com> wrote:
>>>> On 28/03/17 04:46, Robert Haas wrote:
>>>>> On Mon, Mar 27, 2017 at 10:04 PM, Andres Freund <andres@anarazel.de> wrote:
>>>>>>> Btw now that I look at the code, I guess we'll want to get rid of
>>>>>>> bgw_main completely in HEAD given that we can't guarantee it will be
>>>>>>> valid even for shared_preload_library libraries. For older branches I
>>>>>>> would leave things as they are in this regard as there don't seem to be
>>>>>>> any immediate issue for standard binaries.
>>>>>>
>>>>>> As long as you fix it so culicidae is happy (in 9.6) ;).  I think it's
>>>>>> fine to just introduce bgw_builtin_id or such, and leave the bgw_main
>>>>>> code in place in < HEAD.
>>>>>
>>>>> I wasn't thinking of introducing bgw_builtin_id.  My idea was just
>>>>> along the lines of
>>>>>
>>>>> if (bgw_library_name == NULL && bgw_function_name != NULL)
>>>>> {
>>>>>     if (strcmp(bgw_function_name, "ParallelQueryMain") == 0)
>>>>>        ParallelQueryMain(blah);
>>>>>     else if (strcmp(bgw_function_name, "LogicalReplicationMain") == 0)
>>>>>        LogicalReplicationMain(blah);
>>>>> }
>>>>>
>>>>> I think something like that is certainly better for the back-branches,
>>>>> because it doesn't cause an ABI break.  But I think it would also be
>>>>> fine for master.
>>>>>
>>>>
>>>> I had something slightly more complex like the attached in mind.
>>>
>>> Seems broadly reasonable on a quick look, but I think we should leave
>>> bgw_main intact in 9.6.  It may be working for fine for people who
>>> don't care about Windows, and I'd rather not break it gratuitously.
>>> Can we have two patches, one of which converts the internal stuff to
>>> use the new mechanism and a second of which removes bgw_main?  The
>>> second one, at least, also needs to update the documentation.  (A good
>>> practice when removing an identifier is to run 'git grep
>>> thing_i_am_removing' after removing it...)
>>>
>>
>> Yes I agree (and I said it in the thread) I plan to submit the 9.6 too,
>> I just sent what I had before having it all ready mainly because you
>> started talking about code already and I didn't want you to waste time
>> with it needlessly, but it was already 5AM for me ;).
>>
> 
> Okay finally got to it, the 9.6 adds similar mechanism as HEAD (it could
> be simpler there but it seems like same code is better) but does not
> remove the bgw_main.
> 

Sigh, forgot git add for the docs, so one more try...

-- 
  Petr Jelinek                  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services

  26. Re: logical replication launcher crash on buildfarm

    Robert Haas <robertmhaas@gmail.com> — 2017-03-31T13:42:31Z

    On Tue, Mar 28, 2017 at 7:39 PM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:
> Sigh, forgot git add for the docs, so one more try...

+    if (strncmp(worker->bgw_library_name, "postgres", 8) != 0)
+        return NULL;

I think that's not right.  You don't want to match postgresshdkgjsdglkjs.

Aside from that, these look good to me.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  27. Re: logical replication launcher crash on buildfarm

    Petr Jelinek <petr.jelinek@2ndquadrant.com> — 2017-04-01T00:32:41Z

    On 31/03/17 15:42, Robert Haas wrote:
> On Tue, Mar 28, 2017 at 7:39 PM, Petr Jelinek
> <petr.jelinek@2ndquadrant.com> wrote:
>> Sigh, forgot git add for the docs, so one more try...
> 
> +    if (strncmp(worker->bgw_library_name, "postgres", 8) != 0)
> +        return NULL;
> 
> I think that's not right.  You don't want to match postgresshdkgjsdglkjs.
>

Right, changed to BGW_MAXLEN.

> Aside from that, these look good to me.
> 

Cool.

-- 
  Petr Jelinek                  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services

  28. Re: logical replication launcher crash on buildfarm

    Robert Haas <robertmhaas@gmail.com> — 2017-04-01T00:53:04Z

    On Fri, Mar 31, 2017 at 8:32 PM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:
> Right, changed to BGW_MAXLEN.

Hmm, I don't know if there's any good reason not to just use strcmp(),
but sure, OK.  Committed and back-patched.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  29. Re: logical replication launcher crash on buildfarm

    Petr Jelinek <petr.jelinek@2ndquadrant.com> — 2017-04-01T01:26:01Z

    On 01/04/17 02:53, Robert Haas wrote:
> On Fri, Mar 31, 2017 at 8:32 PM, Petr Jelinek
> <petr.jelinek@2ndquadrant.com> wrote:
>> Right, changed to BGW_MAXLEN.
> 
> Hmm, I don't know if there's any good reason not to just use strcmp(),
> but sure, OK.  Committed and back-patched.
> 

Hmm culicidae still fails, this time only in parallel worker code. This
didn't happen on my machine which is strange. Looking at the code, we
are passing the fps->entrypoint as function pointer again so of course
it fails. We have some code to load libraries again but even that gets
initial entrypoint passed as function pointer
(ParallelExtensionTrampoline). I wonder if we'll have to generalize the
InternalBGWorkers even more to some kind of internal function name to
pointer map and add the parallel entry points there as well.

-- 
  Petr Jelinek                  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services

  30. Re: logical replication launcher crash on buildfarm

    Robert Haas <robertmhaas@gmail.com> — 2017-04-01T01:30:17Z

    On Fri, Mar 31, 2017 at 9:26 PM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:
>> Hmm, I don't know if there's any good reason not to just use strcmp(),
>> but sure, OK.  Committed and back-patched.
>
> Hmm culicidae still fails, this time only in parallel worker code. This
> didn't happen on my machine which is strange. Looking at the code, we
> are passing the fps->entrypoint as function pointer again so of course
> it fails. We have some code to load libraries again but even that gets
> initial entrypoint passed as function pointer
> (ParallelExtensionTrampoline). I wonder if we'll have to generalize the
> InternalBGWorkers even more to some kind of internal function name to
> pointer map and add the parallel entry points there as well.

Argh, I forgot about that.  I think we need to use something like
ParallelExtensionTrampoline all the time, not just for libraries.
Since effectively, we've determined that postgres itself has the same
problem.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  31. Re: logical replication launcher crash on buildfarm

    Andres Freund <andres@anarazel.de> — 2017-04-01T01:44:30Z

    On 2017-04-01 03:26:01 +0200, Petr Jelinek wrote:
> On 01/04/17 02:53, Robert Haas wrote:
> > On Fri, Mar 31, 2017 at 8:32 PM, Petr Jelinek
> > <petr.jelinek@2ndquadrant.com> wrote:
> >> Right, changed to BGW_MAXLEN.
> > 
> > Hmm, I don't know if there's any good reason not to just use strcmp(),
> > but sure, OK.  Committed and back-patched.

Cool!


> Hmm culicidae still fails, this time only in parallel worker code.
> This  didn't happen on my machine which is strange.

Interesting.  Did you reproduce the failure before?  I wonder if the
issue is that you don't build a position independent executable?

- Andres

  32. Re: logical replication launcher crash on buildfarm

    Petr Jelinek <petr.jelinek@2ndquadrant.com> — 2017-04-01T01:57:05Z

    On 01/04/17 03:44, Andres Freund wrote:
> On 2017-04-01 03:26:01 +0200, Petr Jelinek wrote:
>> On 01/04/17 02:53, Robert Haas wrote:
>>> On Fri, Mar 31, 2017 at 8:32 PM, Petr Jelinek
>>> <petr.jelinek@2ndquadrant.com> wrote:
>>>> Right, changed to BGW_MAXLEN.
>>>
>>> Hmm, I don't know if there's any good reason not to just use strcmp(),
>>> but sure, OK.  Committed and back-patched.
> 
> Cool!
> 
> 
>> Hmm culicidae still fails, this time only in parallel worker code.
>> This  didn't happen on my machine which is strange.
> 
> Interesting.  Did you reproduce the failure before?  I wonder if the
> issue is that you don't build a position independent executable?
> 

I did reproduce the original issue yes, that's how I tested the patch. I
just copy pasted CFLAGS from the buildfarm configuration of culicidae TBH.

-- 
  Petr Jelinek                  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services

  33. Re: logical replication launcher crash on buildfarm

    Andres Freund <andres@anarazel.de> — 2017-04-01T02:14:28Z

    On 2017-04-01 03:57:05 +0200, Petr Jelinek wrote:
> On 01/04/17 03:44, Andres Freund wrote:
> > On 2017-04-01 03:26:01 +0200, Petr Jelinek wrote:
> >> On 01/04/17 02:53, Robert Haas wrote:
> >>> On Fri, Mar 31, 2017 at 8:32 PM, Petr Jelinek
> >>> <petr.jelinek@2ndquadrant.com> wrote:
> >>>> Right, changed to BGW_MAXLEN.
> >>>
> >>> Hmm, I don't know if there's any good reason not to just use strcmp(),
> >>> but sure, OK.  Committed and back-patched.
> > 
> > Cool!
> > 
> > 
> >> Hmm culicidae still fails, this time only in parallel worker code.
> >> This  didn't happen on my machine which is strange.
> > 
> > Interesting.  Did you reproduce the failure before?  I wonder if the
> > issue is that you don't build a position independent executable?
> > 
> 
> I did reproduce the original issue yes, that's how I tested the patch. I
> just copy pasted CFLAGS from the buildfarm configuration of culicidae TBH.

That's not necessarily sufficient, unless you also use a sufficiently
new debian - they changed gcc to default to PIE being enabled by
default.


Greetings,

Andres Freund

  34. Re: logical replication launcher crash on buildfarm

    Andres Freund <andres@anarazel.de> — 2017-04-01T02:19:28Z

    On 2017-03-31 21:30:17 -0400, Robert Haas wrote:
> On Fri, Mar 31, 2017 at 9:26 PM, Petr Jelinek
> <petr.jelinek@2ndquadrant.com> wrote:
> >> Hmm, I don't know if there's any good reason not to just use strcmp(),
> >> but sure, OK.  Committed and back-patched.
> >
> > Hmm culicidae still fails, this time only in parallel worker code. This
> > didn't happen on my machine which is strange. Looking at the code, we
> > are passing the fps->entrypoint as function pointer again so of course
> > it fails. We have some code to load libraries again but even that gets
> > initial entrypoint passed as function pointer
> > (ParallelExtensionTrampoline). I wonder if we'll have to generalize the
> > InternalBGWorkers even more to some kind of internal function name to
> > pointer map and add the parallel entry points there as well.
> 
> Argh, I forgot about that.  I think we need to use something like
> ParallelExtensionTrampoline all the time, not just for libraries.
> Since effectively, we've determined that postgres itself has the same
> problem.

I wonder if we shouldn't just introduce a KnownFunctionPointer[] map,
that can be used by various facilities (bgworkers themselves,
parallelism, and probably more coming).  Otherwise we'll introduce
mechanisms like 7d8f6986b8 in multiple places.

- Andres

  35. Re: logical replication launcher crash on buildfarm

    Petr Jelinek <petr.jelinek@2ndquadrant.com> — 2017-04-01T02:46:39Z

    On 01/04/17 04:19, Andres Freund wrote:
> On 2017-03-31 21:30:17 -0400, Robert Haas wrote:
>> On Fri, Mar 31, 2017 at 9:26 PM, Petr Jelinek
>> <petr.jelinek@2ndquadrant.com> wrote:
>>>> Hmm, I don't know if there's any good reason not to just use strcmp(),
>>>> but sure, OK.  Committed and back-patched.
>>>
>>> Hmm culicidae still fails, this time only in parallel worker code. This
>>> didn't happen on my machine which is strange. Looking at the code, we
>>> are passing the fps->entrypoint as function pointer again so of course
>>> it fails. We have some code to load libraries again but even that gets
>>> initial entrypoint passed as function pointer
>>> (ParallelExtensionTrampoline). I wonder if we'll have to generalize the
>>> InternalBGWorkers even more to some kind of internal function name to
>>> pointer map and add the parallel entry points there as well.
>>
>> Argh, I forgot about that.  I think we need to use something like
>> ParallelExtensionTrampoline all the time, not just for libraries.
>> Since effectively, we've determined that postgres itself has the same
>> problem.
> 
> I wonder if we shouldn't just introduce a KnownFunctionPointer[] map,
> that can be used by various facilities (bgworkers themselves,
> parallelism, and probably more coming).  Otherwise we'll introduce
> mechanisms like 7d8f6986b8 in multiple places.
> 

Yes that's what I meant with what I said above.

-- 
  Petr Jelinek                  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services

  36. Re: logical replication launcher crash on buildfarm

    Petr Jelinek <petr.jelinek@2ndquadrant.com> — 2017-04-04T18:08:17Z

    So this is what I came up with on plane. Generalized the loading
functionality into load_library_function which that can load either
known postgres functions or call load_external_function.

I am not quite sure if fmgr.c is best place to put it, but I didn't want
to include stuff from executor in bgworker.c. I was thinking about
putting it to dfmgr.c (as that's where load_external_function) but again
seemed weird to including executor there.

As with previous patch, 9.6 will need quite different patch as we need
to keep compatibility there, but since I am traveling I think it's
better to share what I have so far.

-- 
  Petr Jelinek                  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services

  37. Re: logical replication launcher crash on buildfarm

    Robert Haas <robertmhaas@gmail.com> — 2017-04-05T14:09:26Z

    On Tue, Apr 4, 2017 at 2:08 PM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:
> So this is what I came up with on plane. Generalized the loading
> functionality into load_library_function which that can load either
> known postgres functions or call load_external_function.
>
> I am not quite sure if fmgr.c is best place to put it, but I didn't want
> to include stuff from executor in bgworker.c. I was thinking about
> putting it to dfmgr.c (as that's where load_external_function) but again
> seemed weird to including executor there.
>
> As with previous patch, 9.6 will need quite different patch as we need
> to keep compatibility there, but since I am traveling I think it's
> better to share what I have so far.

- * Register a new background worker while processing shared_preload_libraries.
+ * Register a new static background worker.
  *
- * This can only be called in the _PG_init function of a module library
- * that's loaded by shared_preload_libraries; otherwise it has no effect.
+ * This can only be called directly from postmaster or in the _PG_init
+ * function of a module library that's loaded by shared_preload_libraries;
+ * otherwise it will have no effect.

This does seem to need updating, but that's a somewhat separate issue
from fixing the problem at issue.

+         * Serialize extension entrypoint information. It's unsafe to pass
+         * function pointers across parallel processes as the function pointer
+         * may be different in new process in EXEC_BACKEND builds so we
+         * always pass library and function name.

The word "extension" should be removed.

This basically looks OK otherwise, but we'll need something else for 9.6.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  38. Re: logical replication launcher crash on buildfarm

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-04-15T04:01:54Z

    Petr Jelinek <petr.jelinek@2ndquadrant.com> writes:
> So this is what I came up with on plane. Generalized the loading
> functionality into load_library_function which that can load either
> known postgres functions or call load_external_function.

I've had more than enough of seeing buildfarm failures from culicidae,
so I whacked this around until I was happy with it and pushed it.
Further adjustments welcome of course.

> I am not quite sure if fmgr.c is best place to put it, but I didn't want
> to include stuff from executor in bgworker.c.

I really did not care for putting those references into fmgr.c.  Aside
from the layering violations involved, there was a type cheat, in that
you had functions with entirely different signatures in the same list.
I eventually decided that the least ugly solution was to create two
different pointer arrays and frontend functions, one for bgworkers and
one for parallel workers.  That ends up requiring only one extra
export/import, to make ParallelQueryMain accessible in parallel.c.
Maybe we need to rethink the division of labor between parallel.c
and execParallel.c, but that would depend on somebody explaining
what the difference is in the first place.

> As with previous patch, 9.6 will need quite different patch as we need
> to keep compatibility there,

I don't really understand why 9.6 needs a significantly different
patch.  AFAICS, it simply does not work (with any reliability)
for a loadable module to call CreateParallelContext directly in 9.6.
The only thing that actually works is to call the undocumented
CreateParallelContextForExternalFunction.  I suggest that we could
back-patch this as-is, if we add CreateParallelContextForExternalFunction
as a trivial wrapper around CreateParallelContext.

			regards, tom lane

  39. Re: logical replication launcher crash on buildfarm

    Petr Jelinek <petr.jelinek@2ndquadrant.com> — 2017-04-15T14:43:43Z

    On 15/04/17 06:01, Tom Lane wrote:
> Petr Jelinek <petr.jelinek@2ndquadrant.com> writes:
>> So this is what I came up with on plane. Generalized the loading
>> functionality into load_library_function which that can load either
>> known postgres functions or call load_external_function.
> 
> I've had more than enough of seeing buildfarm failures from culicidae,
> so I whacked this around until I was happy with it and pushed it.
> Further adjustments welcome of course.

Thanks. Seems like culicidae is finally happy with master.

> 
>> I am not quite sure if fmgr.c is best place to put it, but I didn't want
>> to include stuff from executor in bgworker.c.
> 
> I really did not care for putting those references into fmgr.c.  Aside
> from the layering violations involved, there was a type cheat, in that
> you had functions with entirely different signatures in the same list.
> I eventually decided that the least ugly solution was to create two
> different pointer arrays and frontend functions, one for bgworkers and
> one for parallel workers.  That ends up requiring only one extra
> export/import, to make ParallelQueryMain accessible in parallel.c.
> Maybe we need to rethink the division of labor between parallel.c
> and execParallel.c, but that would depend on somebody explaining
> what the difference is in the first place.
> 

Sure, I did not really like the placement in fmgr.c either, just could
not find good place that would work well for both. I tend to err on the
less code duplication side of "less ugly", but the different function
signatures is reasonable argument for splitting it as we get additional
compiler check.

>> As with previous patch, 9.6 will need quite different patch as we need
>> to keep compatibility there,
> 
> I don't really understand why 9.6 needs a significantly different
> patch.  AFAICS, it simply does not work (with any reliability)
> for a loadable module to call CreateParallelContext directly in 9.6.
> The only thing that actually works is to call the undocumented
> CreateParallelContextForExternalFunction.  I suggest that we could
> back-patch this as-is, if we add CreateParallelContextForExternalFunction
> as a trivial wrapper around CreateParallelContext.
> 

I think the problem is that if somebody was using CreateParallelContext
it may have worked on unix when just normally forking if the extension
was loaded via shared_preload_libraries. And lot of extensions are linux
only. So we might break working setup for somebody if we change the
function signature.

We may need to keep CreateParallelContext as is in back branches and add
some CreateParallelContextInternal which would do what
CreateParallelContext does in master (and is used by postgres) and then
make CreateParallelContextForExternalFunction simple wrapper around
that. It's somewhat ugly though.

-- 
  Petr Jelinek                  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services

  40. Re: logical replication launcher crash on buildfarm

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-04-15T18:49:42Z

    Petr Jelinek <petr.jelinek@2ndquadrant.com> writes:
> On 15/04/17 06:01, Tom Lane wrote:
>> I've had more than enough of seeing buildfarm failures from culicidae,
>> so I whacked this around until I was happy with it and pushed it.
>> Further adjustments welcome of course.

> Thanks. Seems like culicidae is finally happy with master.

Well, only sort of happy, but I see how we can fix that.

>> I don't really understand why 9.6 needs a significantly different
>> patch.  AFAICS, it simply does not work (with any reliability)
>> for a loadable module to call CreateParallelContext directly in 9.6.

> I think the problem is that if somebody was using CreateParallelContext
> it may have worked on unix when just normally forking if the extension
> was loaded via shared_preload_libraries. And lot of extensions are linux
> only. So we might break working setup for somebody if we change the
> function signature.

Ah, I didn't think of shared_preload_libraries.  OK, we can't change
the signature in released branches.

> We may need to keep CreateParallelContext as is in back branches and add
> some CreateParallelContextInternal which would do what
> CreateParallelContext does in master (and is used by postgres) and then
> make CreateParallelContextForExternalFunction simple wrapper around
> that. It's somewhat ugly though.

Yeah, but ugly compatibility hacks in back branches are pretty common.
But rather than inventing "CreateParallelContextInternal", I suggest
we just have the core code call CreateParallelContextForExternalFunction.

			regards, tom lane

  41. Re: logical replication launcher crash on buildfarm

    Robert Haas <robertmhaas@gmail.com> — 2017-04-16T05:35:46Z

    On Sat, Apr 15, 2017 at 12:01 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Maybe we need to rethink the division of labor between parallel.c
> and execParallel.c, but that would depend on somebody explaining
> what the difference is in the first place.

parallel.c handles general concerns related to any sort of
parallelism, particularly those related to transaction management,
hence it's in src/backend/access/transam. execParallel.c handles
concerns that are specific to the executor, hence it's in
src/backend/executor.  The header comment for execParallel.c is fairly
informative on this point; now that I look at it, parallel.c has only
a stub comment, which really ought to be expanded.  Note that while
parallel *query* uses both, a parallel utility command will likely use
only the facilities in parallel.c, so the distinction is not academic.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company