Обсуждение: [HACKERS] src/test/subscription/t/002_types.pl hanging on particular environment

Hi,

The subscription tests 002_types.pl sometimes hangs for a while and
then times out when run on a Travis CI build VM running Ubuntu Trusty
if --enable-coverage is used.  I guess it might be a timing/race
problem because I can't think of any mechanism specific to coverage
instrumentation except that it slows stuff down, but I don't know.
The only other thing I can think of is that perhaps the instrumented
code is writing something to stdout or stderr and that's finding its
way into some protocol stream and confusing things, but I can't
reproduce this on any of my usual development machines.  I haven't
tried that exact operating system.  Maybe it's a bug in the toolchain,
but that's an Ubuntu LTS release so I assume other people use it
(though I don't see it in the buildfarm).

Example:

t/001_rep_changes.pl .. ok
t/002_types.pl ........ #
# Looks like your test exited with 29 before it could output anything.
t/002_types.pl ........ Dubious, test returned 29 (wstat 7424, 0x1d00)
Failed 3/3 subtests
t/003_constraints.pl .. ok
t/004_sync.pl ......... ok
t/005_encoding.pl ..... ok
t/007_ddl.pl .......... ok
Test Summary Report
-------------------
t/002_types.pl      (Wstat: 7424 Tests: 0 Failed: 0) Non-zero exit status: 29 Parse errors: Bad plan.  You planned 3
testsbut ran 0.
 

Before I figured out that --coverage was relevant, I wondered if the
recent commit 8edacab209957520423770851351ab4013cb0167 which landed
around the time I spotted this might have something to do with it, but
I tried reverting the code change in there and it didn't help.  Here's
a build log:
   https://travis-ci.org/macdice/postgres/jobs/276752803

As you can see the script used was:
   ./configure --enable-debug --enable-cassert --enable-tap-tests
--enable-coverage && make -j4 all contrib && make -C
src/test/subscription check

In this build you can see the output of the following at the end,
which might provide clues to the initiated.  You might need to click a
small triangle to unfold the commands' output.
   cat ./src/test/subscription/tmp_check/log/002_types_publisher.log   cat
./src/test/subscription/tmp_check/log/002_types_subscriber.log  cat
./src/test/subscription/tmp_check/log/regress_log_002_types

There are messages like this:
   WARNING:  terminating connection because of crash of another server process   DETAIL:  The postmaster has commanded
thisserver process to roll
 
back the current transaction and exit, because another server process
exited abnormally and possibly corrupted shared memory.   HINT:  In a moment you should be able to reconnect to the
database
and repeat your command.

As far as I know these are misleading, it's really just an immediate
shutdown.  There is no core file, so I don't believe anything actually
crashed.

Here's a version that's the same except it doesn't have
--enable-coverage.  It passes:
   https://travis-ci.org/macdice/postgres/jobs/276771654

Any ideas?

-- 
Thomas Munro
http://www.enterprisedb.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hi,

On 2017-09-18 21:57:04 +1200, Thomas Munro wrote:
> The subscription tests 002_types.pl sometimes hangs for a while and
> then times out when run on a Travis CI build VM running Ubuntu Trusty
> if --enable-coverage is used.

Yea, I saw that too.


> I guess it might be a timing/race
> problem because I can't think of any mechanism specific to coverage
> instrumentation except that it slows stuff down, but I don't know.
> The only other thing I can think of is that perhaps the instrumented
> code is writing something to stdout or stderr and that's finding its
> way into some protocol stream and confusing things, but I can't
> reproduce this on any of my usual development machines.  I haven't
> tried that exact operating system.  Maybe it's a bug in the toolchain,
> but that's an Ubuntu LTS release so I assume other people use it
> (though I don't see it in the buildfarm).

I've run this locally through a number of iterations with coverage
enabled, I think I reproduced it once, but unfortunately I'd continued
because I was working on something else at that moment.


It might be worthwhile to play around with replacing the or die in
my $synced_query =
"SELECT count(1) = 0 FROM pg_subscription_rel WHERE srsubstate NOT IN ('s', 'r');";
$node_subscriber->poll_query_until('postgres', $synced_query) or die "Timed out while waiting for subscriber to
synchronizedata";
 
with something like
or diag($node_subscriber->safe_psql('postgres', 'SELECT * FROM pg_subscription_rel')
just to know where to go from here.


>     WARNING:  terminating connection because of crash of another server process
>     DETAIL:  The postmaster has commanded this server process to roll
> back the current transaction and exit, because another server process
> exited abnormally and possibly corrupted shared memory.
>     HINT:  In a moment you should be able to reconnect to the database
> and repeat your command.
> 
> As far as I know these are misleading, it's really just an immediate
> shutdown.  There is no core file, so I don't believe anything actually
> crashed.

I was about to complain about these, for entirely unrelated reasons. I
think it's a bad idea - and there's a couple complains on the lists too,
to emit these warnings.  It's not entirely trivial to fix though :(

Greetings,

Andres Freund


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Thomas Munro <thomas.munro@enterprisedb.com> writes:
> In this build you can see the output of the following at the end,
> which might provide clues to the initiated.  You might need to click a
> small triangle to unfold the commands' output.

>     cat ./src/test/subscription/tmp_check/log/002_types_publisher.log
>     cat ./src/test/subscription/tmp_check/log/002_types_subscriber.log
>     cat ./src/test/subscription/tmp_check/log/regress_log_002_types

The subscriber log includes
2017-09-18 08:43:08.240 UTC [15672] WARNING:  out of background worker slots
2017-09-18 08:43:08.240 UTC [15672] HINT:  You might need to increase max_worker_processes.

Maybe that's harmless, but I'm suspicious that it's a smoking gun.
I think perhaps this reflects a failed attempt to launch a worker,
which the caller does not realize has failed to launch because of the
lack of worker-fork-failure error recovery I bitched about months ago
[1], leading to subscription startup waiting forever for a worker that's
never going to report finishing.

I see Amit K. just posted a patch in that area [2], haven't looked at it
yet.
        regards, tom lane

[1] https://www.postgresql.org/message-id/4905.1492813727@sss.pgh.pa.us
[2] https://www.postgresql.org/message-id/CAA4eK1KDfKkvrjxsKJi3WPyceVi3dH1VCkbTJji2fuwKuB=3uw@mail.gmail.com


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

I wrote:
> The subscriber log includes
> 2017-09-18 08:43:08.240 UTC [15672] WARNING:  out of background worker slots
> 2017-09-18 08:43:08.240 UTC [15672] HINT:  You might need to increase max_worker_processes.

> Maybe that's harmless, but I'm suspicious that it's a smoking gun.

Actually: looking at the place where this is issued, namely
logicalrep_worker_launch, and comparing it to the cleanup logic
in WaitForReplicationWorkerAttach, it looks to me like the problem
is we're failing to do logicalrep_worker_cleanup() in this case.
We have initialized a logical rep worker slot, and we're just
walking away from it.

I'll go see if I can't reproduce this by injecting random failures right
there.
        regards, tom lane


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, Sep 18, 2017 at 7:46 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Thomas Munro <thomas.munro@enterprisedb.com> writes:
>> In this build you can see the output of the following at the end,
>> which might provide clues to the initiated.  You might need to click a
>> small triangle to unfold the commands' output.
>
>>     cat ./src/test/subscription/tmp_check/log/002_types_publisher.log
>>     cat ./src/test/subscription/tmp_check/log/002_types_subscriber.log
>>     cat ./src/test/subscription/tmp_check/log/regress_log_002_types
>
> The subscriber log includes
> 2017-09-18 08:43:08.240 UTC [15672] WARNING:  out of background worker slots
> 2017-09-18 08:43:08.240 UTC [15672] HINT:  You might need to increase max_worker_processes.
>
> Maybe that's harmless, but I'm suspicious that it's a smoking gun.
> I think perhaps this reflects a failed attempt to launch a worker,
> which the caller does not realize has failed to launch because of the
> lack of worker-fork-failure error recovery I bitched about months ago
> [1], leading to subscription startup waiting forever for a worker that's
> never going to report finishing.
>
> I see Amit K. just posted a patch in that area [2], haven't looked at it
> yet.
>

I have noticed while fixing the issue you are talking that this path
is also susceptible to such problems.  In
WaitForReplicationWorkerAttach(), it relies on
GetBackgroundWorkerPid() to know the status of the worker which won't
give the correct status in case of fork failure.  The patch I have
posted has a fix for the issue, however, this could be an entirely
different issue altogether as it appears from your next email in this
thread.


[1] - https://www.postgresql.org/message-id/CAA4eK1KDfKkvrjxsKJi3WPyceVi3dH1VCkbTJji2fuwKuB%3D3uw%40mail.gmail.com

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Amit Kapila <amit.kapila16@gmail.com> writes:
> On Mon, Sep 18, 2017 at 7:46 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> The subscriber log includes
>> 2017-09-18 08:43:08.240 UTC [15672] WARNING:  out of background worker slots
>> Maybe that's harmless, but I'm suspicious that it's a smoking gun.

> I have noticed while fixing the issue you are talking that this path
> is also susceptible to such problems.  In
> WaitForReplicationWorkerAttach(), it relies on
> GetBackgroundWorkerPid() to know the status of the worker which won't
> give the correct status in case of fork failure.  The patch I have
> posted has a fix for the issue,

Your GetBackgroundWorkerPid fix looks good as far as it goes, but
I feel that WaitForReplicationWorkerAttach's problems go way deeper
than that --- in fact, that function should not exist at all IMO.

The way that launcher.c is set up, it's relying on either the calling
process or the called process to free the logicalrep slot when done.
The called process might never exist at all, so the second half of
that is obviously unreliable; but WaitForReplicationWorkerAttach
can hardly be relied on either, seeing it's got a big fat exit-on-
SIGINT in it.  I thought about putting a PG_TRY, or more likely
PG_ENSURE_ERROR_CLEANUP, into it, but that doesn't fix the basic
problem: you can't assume that the worker isn't ever going to start,
just because you got a signal that means you shouldn't wait anymore.

Now, this rickety business might be fine if it were only about the
states of the caller and called processes, but we've got long-lived
shared data involved (ie the worker slots); failing to clean it up
is not an acceptable outcome.

So, frankly, I think we would be best off losing the "logical rep
worker slot" business altogether, and making do with just bgworker
slots.  The alternative is getting the postmaster involved in cleaning
up lrep slots as well as bgworker slots, and I'm going to resist
any such idea strongly.  That way madness lies, or at least an
unreliable postmaster --- if we need lrep slots, what other forty-seven
data structures are we going to need the postmaster to clean up
in the future?

I haven't studied the code enough to know why it grew lrep worker
slots in the first place.  Maybe someone could explain?
        regards, tom lane


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

n 18/09/17 18:42, Tom Lane wrote:
> Amit Kapila <amit.kapila16@gmail.com> writes:
>> On Mon, Sep 18, 2017 at 7:46 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> The subscriber log includes
>>> 2017-09-18 08:43:08.240 UTC [15672] WARNING:  out of background worker slots
>>> Maybe that's harmless, but I'm suspicious that it's a smoking gun.
> 
>> I have noticed while fixing the issue you are talking that this path
>> is also susceptible to such problems.  In
>> WaitForReplicationWorkerAttach(), it relies on
>> GetBackgroundWorkerPid() to know the status of the worker which won't
>> give the correct status in case of fork failure.  The patch I have
>> posted has a fix for the issue,
> 
> Your GetBackgroundWorkerPid fix looks good as far as it goes, but
> I feel that WaitForReplicationWorkerAttach's problems go way deeper
> than that --- in fact, that function should not exist at all IMO.
> 
> The way that launcher.c is set up, it's relying on either the calling
> process or the called process to free the logicalrep slot when done.
> The called process might never exist at all, so the second half of
> that is obviously unreliable; but WaitForReplicationWorkerAttach
> can hardly be relied on either, seeing it's got a big fat exit-on-
> SIGINT in it.  I thought about putting a PG_TRY, or more likely
> PG_ENSURE_ERROR_CLEANUP, into it, but that doesn't fix the basic
> problem: you can't assume that the worker isn't ever going to start,
> just because you got a signal that means you shouldn't wait anymore.
> 
> Now, this rickety business might be fine if it were only about the
> states of the caller and called processes, but we've got long-lived
> shared data involved (ie the worker slots); failing to clean it up
> is not an acceptable outcome.
> 

We'll clean it up eventually if somebody requests creation of new
logical replication worker and that slot is needed. See the "garbage
collection" part in logicalrep_worker_launch(). I know it's not ideal
solution, but it's the best one I could think of given the bellow.

> So, frankly, I think we would be best off losing the "logical rep
> worker slot" business altogether, and making do with just bgworker
> slots.  The alternative is getting the postmaster involved in cleaning
> up lrep slots as well as bgworker slots, and I'm going to resist
> any such idea strongly.  That way madness lies, or at least an
> unreliable postmaster --- if we need lrep slots, what other forty-seven
> data structures are we going to need the postmaster to clean up
> in the future?
> 
> I haven't studied the code enough to know why it grew lrep worker
> slots in the first place.  Maybe someone could explain?
> 

I am not quite sure I understand this question, we need to store
additional info about workers in shared memory so we need slots for that.

If you are asking why they are not identified by the
BackgroundWorkerHandle, then it's because it's private struct and can't
be shared with other processes so there is no way to link the logical
worker info with bgworker directly. I mentioned that I am not happy
about this during the original development but it didn't gather any
attention which I took to mean that nobody minds.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Tue, Sep 19, 2017 at 3:34 PM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:
> n 18/09/17 18:42, Tom Lane wrote:
>
>> So, frankly, I think we would be best off losing the "logical rep
>> worker slot" business altogether, and making do with just bgworker
>> slots.

I think that would be cleaner as compared to what we have now.

>>  The alternative is getting the postmaster involved in cleaning
>> up lrep slots as well as bgworker slots, and I'm going to resist
>> any such idea strongly.  That way madness lies, or at least an
>> unreliable postmaster --- if we need lrep slots, what other forty-seven
>> data structures are we going to need the postmaster to clean up
>> in the future?
>>
>> I haven't studied the code enough to know why it grew lrep worker
>> slots in the first place.  Maybe someone could explain?
>>
>
> I am not quite sure I understand this question, we need to store
> additional info about workers in shared memory so we need slots for that.
>

Yeah, but you could have used the way we do for parallel query where
we setup dsm and share all such information.  You can check the logic
of execparallel.c and parallel.c to see how we do all such stuff for
parallel query.


-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 19/09/17 14:33, Amit Kapila wrote:
> On Tue, Sep 19, 2017 at 3:34 PM, Petr Jelinek
> <petr.jelinek@2ndquadrant.com> wrote:
>> n 18/09/17 18:42, Tom Lane wrote:
>>
>>> So, frankly, I think we would be best off losing the "logical rep
>>> worker slot" business altogether, and making do with just bgworker
>>> slots.
> 
> I think that would be cleaner as compared to what we have now.
> 
>>>  The alternative is getting the postmaster involved in cleaning
>>> up lrep slots as well as bgworker slots, and I'm going to resist
>>> any such idea strongly.  That way madness lies, or at least an
>>> unreliable postmaster --- if we need lrep slots, what other forty-seven
>>> data structures are we going to need the postmaster to clean up
>>> in the future?
>>>
>>> I haven't studied the code enough to know why it grew lrep worker
>>> slots in the first place.  Maybe someone could explain?
>>>
>>
>> I am not quite sure I understand this question, we need to store
>> additional info about workers in shared memory so we need slots for that.
>>
> 
> Yeah, but you could have used the way we do for parallel query where
> we setup dsm and share all such information.  You can check the logic
> of execparallel.c and parallel.c to see how we do all such stuff for
> parallel query.
> 

I don't understand how DSM helps in this case (except perhaps the memory
allocation being dynamic rather than static). We still need this shared
memory area to be accessible from other places than launcher (the
paralllel query has one lead which manages everything, that's not true
for logical replication) and we need it to survive restart of launcher
for example, so all the current issues will stay.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Tue, Sep 19, 2017 at 6:29 PM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:
> On 19/09/17 14:33, Amit Kapila wrote:
>> On Tue, Sep 19, 2017 at 3:34 PM, Petr Jelinek
>> <petr.jelinek@2ndquadrant.com> wrote:
>>> n 18/09/17 18:42, Tom Lane wrote:
>>>
>>>> So, frankly, I think we would be best off losing the "logical rep
>>>> worker slot" business altogether, and making do with just bgworker
>>>> slots.
>>
>> I think that would be cleaner as compared to what we have now.
>>
>>>>  The alternative is getting the postmaster involved in cleaning
>>>> up lrep slots as well as bgworker slots, and I'm going to resist
>>>> any such idea strongly.  That way madness lies, or at least an
>>>> unreliable postmaster --- if we need lrep slots, what other forty-seven
>>>> data structures are we going to need the postmaster to clean up
>>>> in the future?
>>>>
>>>> I haven't studied the code enough to know why it grew lrep worker
>>>> slots in the first place.  Maybe someone could explain?
>>>>
>>>
>>> I am not quite sure I understand this question, we need to store
>>> additional info about workers in shared memory so we need slots for that.
>>>
>>
>> Yeah, but you could have used the way we do for parallel query where
>> we setup dsm and share all such information.  You can check the logic
>> of execparallel.c and parallel.c to see how we do all such stuff for
>> parallel query.
>>
>
> I don't understand how DSM helps in this case (except perhaps the memory
> allocation being dynamic rather than static). We still need this shared
> memory area to be accessible from other places than launcher (the
> paralllel query has one lead which manages everything, that's not true
> for logical replication)
>

I am not much aware of this area.  Can you explain what other usages
it has apart from in the process that has launched the worker and in
worker itself?

> and we need it to survive restart of launcher
> for example, so all the current issues will stay.
>

Do you mean to say that you want to save this part of shared memory
across restart of launcher?

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 19/09/17 15:08, Amit Kapila wrote:
> On Tue, Sep 19, 2017 at 6:29 PM, Petr Jelinek
> <petr.jelinek@2ndquadrant.com> wrote:
>> On 19/09/17 14:33, Amit Kapila wrote:
>>> On Tue, Sep 19, 2017 at 3:34 PM, Petr Jelinek
>>> <petr.jelinek@2ndquadrant.com> wrote:
>>>> n 18/09/17 18:42, Tom Lane wrote:
>>>>
>>>>> So, frankly, I think we would be best off losing the "logical rep
>>>>> worker slot" business altogether, and making do with just bgworker
>>>>> slots.
>>>
>>> I think that would be cleaner as compared to what we have now.
>>>
>>>>>  The alternative is getting the postmaster involved in cleaning
>>>>> up lrep slots as well as bgworker slots, and I'm going to resist
>>>>> any such idea strongly.  That way madness lies, or at least an
>>>>> unreliable postmaster --- if we need lrep slots, what other forty-seven
>>>>> data structures are we going to need the postmaster to clean up
>>>>> in the future?
>>>>>
>>>>> I haven't studied the code enough to know why it grew lrep worker
>>>>> slots in the first place.  Maybe someone could explain?
>>>>>
>>>>
>>>> I am not quite sure I understand this question, we need to store
>>>> additional info about workers in shared memory so we need slots for that.
>>>>
>>>
>>> Yeah, but you could have used the way we do for parallel query where
>>> we setup dsm and share all such information.  You can check the logic
>>> of execparallel.c and parallel.c to see how we do all such stuff for
>>> parallel query.
>>>
>>
>> I don't understand how DSM helps in this case (except perhaps the memory
>> allocation being dynamic rather than static). We still need this shared
>> memory area to be accessible from other places than launcher (the
>> paralllel query has one lead which manages everything, that's not true
>> for logical replication)
>>
> 
> I am not much aware of this area.  Can you explain what other usages
> it has apart from in the process that has launched the worker and in
> worker itself?
> 

The subscription "DDL" commands and monitoring functions need access to
that info. Note that the synchronization workers are not even started by
launcher (I experimented with doing that but it slows the process of
synchronization quite considerably) so it can't manage them unless the
handle is accessible via IPC.

>> and we need it to survive restart of launcher
>> for example, so all the current issues will stay.
>>
> 
> Do you mean to say that you want to save this part of shared memory
> across restart of launcher?
> 

Yes. There is no reason why replication should stop because of launcher
restart.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Tue, Sep 19, 2017 at 3:34 PM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:
> n 18/09/17 18:42, Tom Lane wrote:
>> Amit Kapila <amit.kapila16@gmail.com> writes:
>>> On Mon, Sep 18, 2017 at 7:46 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>>> The subscriber log includes
>>>> 2017-09-18 08:43:08.240 UTC [15672] WARNING:  out of background worker slots
>>>> Maybe that's harmless, but I'm suspicious that it's a smoking gun.
>>
>>> I have noticed while fixing the issue you are talking that this path
>>> is also susceptible to such problems.  In
>>> WaitForReplicationWorkerAttach(), it relies on
>>> GetBackgroundWorkerPid() to know the status of the worker which won't
>>> give the correct status in case of fork failure.  The patch I have
>>> posted has a fix for the issue,
>>
>> Your GetBackgroundWorkerPid fix looks good as far as it goes, but
>> I feel that WaitForReplicationWorkerAttach's problems go way deeper
>> than that --- in fact, that function should not exist at all IMO.
>>
>> The way that launcher.c is set up, it's relying on either the calling
>> process or the called process to free the logicalrep slot when done.
>> The called process might never exist at all, so the second half of
>> that is obviously unreliable; but WaitForReplicationWorkerAttach
>> can hardly be relied on either, seeing it's got a big fat exit-on-
>> SIGINT in it.  I thought about putting a PG_TRY, or more likely
>> PG_ENSURE_ERROR_CLEANUP, into it, but that doesn't fix the basic
>> problem: you can't assume that the worker isn't ever going to start,
>> just because you got a signal that means you shouldn't wait anymore.
>>
>> Now, this rickety business might be fine if it were only about the
>> states of the caller and called processes, but we've got long-lived
>> shared data involved (ie the worker slots); failing to clean it up
>> is not an acceptable outcome.
>>
>
> We'll clean it up eventually if somebody requests creation of new
> logical replication worker and that slot is needed. See the "garbage
> collection" part in logicalrep_worker_launch(). I know it's not ideal
> solution, but it's the best one I could think of given the bellow.
>
>> So, frankly, I think we would be best off losing the "logical rep
>> worker slot" business altogether, and making do with just bgworker
>> slots.  The alternative is getting the postmaster involved in cleaning
>> up lrep slots as well as bgworker slots, and I'm going to resist
>> any such idea strongly.  That way madness lies, or at least an
>> unreliable postmaster --- if we need lrep slots, what other forty-seven
>> data structures are we going to need the postmaster to clean up
>> in the future?
>>
>> I haven't studied the code enough to know why it grew lrep worker
>> slots in the first place.  Maybe someone could explain?
>>
>
> I am not quite sure I understand this question, we need to store
> additional info about workers in shared memory so we need slots for that.
>
> If you are asking why they are not identified by the
> BackgroundWorkerHandle, then it's because it's private struct and can't
> be shared with other processes so there is no way to link the logical
> worker info with bgworker directly.
>

Not sure, but can't we identify the actual worker slot if we just have
pid of background worker?  IIUC, you need access to
BackgroundWorkerHandle by other processes, so that you can stop them
like in case of drop subscription command.  If so, then, might be
storing pid can serve the purpose.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Tue, Sep 19, 2017 at 6:51 PM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:
> On 19/09/17 15:08, Amit Kapila wrote:
>>
>> I am not much aware of this area.  Can you explain what other usages
>> it has apart from in the process that has launched the worker and in
>> worker itself?
>>
>
> The subscription "DDL" commands and monitoring functions need access to
> that info.
>

Got your point, but still not sure if we need to maintain additional
information to replicate something similar to bgworker machinery.


-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 19/09/17 16:30, Amit Kapila wrote:
> On Tue, Sep 19, 2017 at 3:34 PM, Petr Jelinek
> <petr.jelinek@2ndquadrant.com> wrote:
>> n 18/09/17 18:42, Tom Lane wrote:
>>> Amit Kapila <amit.kapila16@gmail.com> writes:
>>>> On Mon, Sep 18, 2017 at 7:46 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> So, frankly, I think we would be best off losing the "logical rep
>>> worker slot" business altogether, and making do with just bgworker
>>> slots.  The alternative is getting the postmaster involved in cleaning
>>> up lrep slots as well as bgworker slots, and I'm going to resist
>>> any such idea strongly.  That way madness lies, or at least an
>>> unreliable postmaster --- if we need lrep slots, what other forty-seven
>>> data structures are we going to need the postmaster to clean up
>>> in the future?
>>>
>>> I haven't studied the code enough to know why it grew lrep worker
>>> slots in the first place.  Maybe someone could explain?
>>>
>>
>> I am not quite sure I understand this question, we need to store
>> additional info about workers in shared memory so we need slots for that.
>>
>> If you are asking why they are not identified by the
>> BackgroundWorkerHandle, then it's because it's private struct and can't
>> be shared with other processes so there is no way to link the logical
>> worker info with bgworker directly.
>>
> 
> Not sure, but can't we identify the actual worker slot if we just have
> pid of background worker?  IIUC, you need access to
> BackgroundWorkerHandle by other processes, so that you can stop them
> like in case of drop subscription command.  If so, then, might be
> storing pid can serve the purpose.
> 

I don't think that pid can be trusted to belong to same process between
the calls, if we could we would not need BackgroundWorkerHandle.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, Sep 18, 2017 at 10:18 PM, Andres Freund <andres@anarazel.de> wrote:
> On 2017-09-18 21:57:04 +1200, Thomas Munro wrote:
>>     WARNING:  terminating connection because of crash of another server process
>>     DETAIL:  The postmaster has commanded this server process to roll
>> back the current transaction and exit, because another server process
>> exited abnormally and possibly corrupted shared memory.
>>     HINT:  In a moment you should be able to reconnect to the database
>> and repeat your command.
>>
>> As far as I know these are misleading, it's really just an immediate
>> shutdown.  There is no core file, so I don't believe anything actually
>> crashed.
>
> I was about to complain about these, for entirely unrelated reasons. I
> think it's a bad idea - and there's a couple complains on the lists too,
> to emit these warnings.  It's not entirely trivial to fix though :(

This type of violent shutdown seems to be associated with occasional
corruption of .gcda files (the files output by GCC coverage builds).
The symptoms are that if you use --enable-coverage and make
check-world you'll very occasionally get a spurious TAP test failure
like this:

#   Failed test 'pg_ctl start: no stderr'
#   at /home/travis/build/postgresql-cfbot/postgresql/src/bin/pg_ctl/../../../src/test/perl/TestLib.pm
line 301.
#          got:
'profiling:/home/travis/build/postgresql-cfbot/postgresql/src/backend/nodes/copyfuncs.gcda:Merge
mismatch for function 94
# '
#     expected: ''

I'm not sure of the exact mechanism though.  GCC supplies a function
__gcov_flush() that normally runs at exit or execve, so if you're
killed without reaching those you don't get any .gcda data.  Perhaps
we are in exit (or fork/exec) partway through writing out coverage
data in __gcov_flush(), and at that moment we are killed.  Then a
subsequent run of instrumented code will find the half-written file
and print the "Merge mismatch" message.

-- 
Thomas Munro
http://www.enterprisedb.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Thomas Munro <thomas.munro@enterprisedb.com> writes:
> This type of violent shutdown seems to be associated with occasional
> corruption of .gcda files (the files output by GCC coverage builds).
> The symptoms are that if you use --enable-coverage and make
> check-world you'll very occasionally get a spurious TAP test failure
> like this:

> #   Failed test 'pg_ctl start: no stderr'
> #   at /home/travis/build/postgresql-cfbot/postgresql/src/bin/pg_ctl/../../../src/test/perl/TestLib.pm
> line 301.
> #          got:
> 'profiling:/home/travis/build/postgresql-cfbot/postgresql/src/backend/nodes/copyfuncs.gcda:Merge
> mismatch for function 94
> # '
> #     expected: ''

> I'm not sure of the exact mechanism though.  GCC supplies a function
> __gcov_flush() that normally runs at exit or execve, so if you're
> killed without reaching those you don't get any .gcda data.  Perhaps
> we are in exit (or fork/exec) partway through writing out coverage
> data in __gcov_flush(), and at that moment we are killed.  Then a
> subsequent run of instrumented code will find the half-written file
> and print the "Merge mismatch" message.

On a slow/loaded machine, perhaps it could be that the postmaster loses
patience and SIGKILLs a backend that's still writing its .gcda data?
If so, maybe we could make SIGKILL_CHILDREN_AFTER_SECS longer in
coverage builds?  Or bite the bullet and make it configurable ...
        regards, tom lane


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2017-09-19 17:20:49 -0400, Tom Lane wrote:
> Thomas Munro <thomas.munro@enterprisedb.com> writes:
> > This type of violent shutdown seems to be associated with occasional
> > corruption of .gcda files (the files output by GCC coverage builds).
> > The symptoms are that if you use --enable-coverage and make
> > check-world you'll very occasionally get a spurious TAP test failure
> > like this:
> 
> > #   Failed test 'pg_ctl start: no stderr'
> > #   at /home/travis/build/postgresql-cfbot/postgresql/src/bin/pg_ctl/../../../src/test/perl/TestLib.pm
> > line 301.
> > #          got:
> > 'profiling:/home/travis/build/postgresql-cfbot/postgresql/src/backend/nodes/copyfuncs.gcda:Merge
> > mismatch for function 94
> > # '
> > #     expected: ''
> 
> > I'm not sure of the exact mechanism though.  GCC supplies a function
> > __gcov_flush() that normally runs at exit or execve, so if you're
> > killed without reaching those you don't get any .gcda data.  Perhaps
> > we are in exit (or fork/exec) partway through writing out coverage
> > data in __gcov_flush(), and at that moment we are killed.  Then a
> > subsequent run of instrumented code will find the half-written file
> > and print the "Merge mismatch" message.

Note that newer gcc's (7+) have a feature to avoid such corruption, by
renaming the files atomically. Possibly the fix here is to just upgrade
to such a version...

Greetings,

Andres Freund


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Craig Ringer <craig@2ndquadrant.com> writes:
> On 19 September 2017 at 18:04, Petr Jelinek <petr.jelinek@2ndquadrant.com>
> wrote:
>> If you are asking why they are not identified by the
>> BackgroundWorkerHandle, then it's because it's private struct and can't
>> be shared with other processes so there is no way to link the logical
>> worker info with bgworker directly.

> I really want BackgroundWorkerHandle to be public, strong +1 from me.

I'm confused about what you think that would accomplish.  AFAICS, the
point of BackgroundWorkerHandle is to allow the creator/requestor of
a bgworker to verify whether or not the slot in question is still
"owned" by its request.  This is necessarily not useful to any other
process, since they didn't make the request.

The thought I had in mind upthread was to get rid of logicalrep slots
in favor of expanding the underlying bgworker slot with some additional
fields that would carry whatever extra info we need about a logicalrep
worker.  Such fields could also be repurposed for additional info about
other kinds of bgworkers, when (not if) we find out we need that.
        regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, Sep 20, 2017 at 9:23 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Craig Ringer <craig@2ndquadrant.com> writes:
>> On 19 September 2017 at 18:04, Petr Jelinek <petr.jelinek@2ndquadrant.com>
>> wrote:
>>> If you are asking why they are not identified by the
>>> BackgroundWorkerHandle, then it's because it's private struct and can't
>>> be shared with other processes so there is no way to link the logical
>>> worker info with bgworker directly.
>
>> I really want BackgroundWorkerHandle to be public, strong +1 from me.
>
> I'm confused about what you think that would accomplish.  AFAICS, the
> point of BackgroundWorkerHandle is to allow the creator/requestor of
> a bgworker to verify whether or not the slot in question is still
> "owned" by its request.
>

Right, but can we avoid maintaining additional information (in_use,
generation,..) in LogicalRepWorker which is similar to bgworker worker
machinery (which in turn can also avoid all the housekeeping for those
variables) if we have access to BackgroundWorkerHandle?


-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Craig Ringer wrote:

> IIRC when we did something similar in pglogical we ran into problems with
> (a) inability to handle an ERROR in a bgworker without exiting and being
> restarted by the postmaster;

I don't understand why this would be; surely you can just setup another
longjmp block for elog(ERROR) to jump to?

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 20/09/17 05:53, Tom Lane wrote:
> Craig Ringer <craig@2ndquadrant.com> writes:
>> On 19 September 2017 at 18:04, Petr Jelinek <petr.jelinek@2ndquadrant.com>
>> wrote:
>>> If you are asking why they are not identified by the
>>> BackgroundWorkerHandle, then it's because it's private struct and can't
>>> be shared with other processes so there is no way to link the logical
>>> worker info with bgworker directly.
> 
>> I really want BackgroundWorkerHandle to be public, strong +1 from me.
> 
> I'm confused about what you think that would accomplish.  AFAICS, the
> point of BackgroundWorkerHandle is to allow the creator/requestor of
> a bgworker to verify whether or not the slot in question is still
> "owned" by its request.  This is necessarily not useful to any other
> process, since they didn't make the request.

I works pretty well as unique identifier of the bgworker which can be
used to check if the specific bgworker process is the one we think it is
and that would work even across processes if it could be shared.
Bgworker slots don't help with that as they can be replaced by different
worker between calls, same goes for PID. It technically does not have to
be public struct, just serialize-able/copyable so knowing size of the
struct ought to be enough for that.

> The thought I had in mind upthread was to get rid of logicalrep slots
> in favor of expanding the underlying bgworker slot with some additional
> fields that would carry whatever extra info we need about a logicalrep
> worker.  Such fields could also be repurposed for additional info about
> other kinds of bgworkers, when (not if) we find out we need that.
> 

Well, that would be nice but those slots are in shared memory so it's
not exactly easy to allow other modules to add arbitrary data there.
Maybe we could add just one field that holds DSM handle (or DSA handle)
to whatever dynamic shared memory is allocated for that slot. I don't
know enough about inner working of DSM to say for sure if that's
feasible or not though.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers