Thread: Assertion failure on PG15 with modified test_shm_mq test

On Wed, Aug 17, 2022 at 11:15:28AM +0530, Pavan Deolasee wrote:
> I notice that pgstat_shutdown_hook() is registered as a before-shmem-exit
> callback. The callback is responsible for detaching from the pgstat shared
> memory segment. But looks like other parts of the system still expect it to
> be available during later stages of proc exit.
>
> It's not clear to me if pgstat shutdown should happen later or code that
> gets executed later in the cycle should not try to use pgstat. It's also
> entirely possible that my usage of SharedFileSet is completely wrong. If
> that's the case, please let me know the mistake in the usage.

That's visibly an issue with shared memory and the stats.  I have
added an open item.  Andres?
--
Michael

Hi,

On 2022-08-17 11:15:28 +0530, Pavan Deolasee wrote:
> I've a slightly modified version of test_shm_mq, that I changed to include
> a shared fileset.  The motivation to do that came because I hit an
> assertion failure with PG15 while doing some development work on BDR and I
> suspected it to be a PG15 bug.

> I notice that pgstat_shutdown_hook() is registered as a before-shmem-exit
> callback. The callback is responsible for detaching from the pgstat shared
> memory segment. But looks like other parts of the system still expect it to
> be available during later stages of proc exit.

> It's not clear to me if pgstat shutdown should happen later or code that
> gets executed later in the cycle should not try to use pgstat. It's also
> entirely possible that my usage of SharedFileSet is completely wrong. If
> that's the case, please let me know the mistake in the usage.

I don't think we have the infrastructure for a nice solution to this at the
moment - we need a fairly large overhaul of process initialization / shutdown
to handle these interdependencies nicely.

We can't move pgstat shutdown into on_dsm callback because that's too late to
allocate *new* dsm segments, which we might need to do while flushing
out pending stats.

See https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=fa91d4c91f28f4819dc54f93adbd413a685e366a
for a way to avoid the problem.

Greetings,

Andres Freund

Hi,

On 2022-08-17 15:02:28 +0900, Michael Paquier wrote:
> On Wed, Aug 17, 2022 at 11:15:28AM +0530, Pavan Deolasee wrote:
> > I notice that pgstat_shutdown_hook() is registered as a before-shmem-exit
> > callback. The callback is responsible for detaching from the pgstat shared
> > memory segment. But looks like other parts of the system still expect it to
> > be available during later stages of proc exit.
> > 
> > It's not clear to me if pgstat shutdown should happen later or code that
> > gets executed later in the cycle should not try to use pgstat. It's also
> > entirely possible that my usage of SharedFileSet is completely wrong. If
> > that's the case, please let me know the mistake in the usage.
> 
> That's visibly an issue with shared memory and the stats.  I have
> added an open item.  Andres?

I don't think there's anything reasonably done about this for 15, as explained
upthread. We need a big redesign of the shutdown sequence at some point, but
...

Greetings,

Andres Freund

At Thu, 18 Aug 2022 16:58:24 +0530, Pavan Deolasee <pavan.deolasee@gmail.com> wrote in 
> Hi,
> 
> On Thu, Aug 18, 2022 at 5:38 AM Andres Freund <andres@anarazel.de> wrote:
> 
> > We can't move pgstat shutdown into on_dsm callback because that's too late
> > to
> > allocate *new* dsm segments, which we might need to do while flushing
> > out pending stats.
> >
> > See
> > https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=fa91d4c91f28f4819dc54f93adbd413a685e366a
> > for a way to avoid the problem.
> >
> >
> Thanks for the hint. I will try that approach. I wonder though if there is
> something more we can do. For example, would it make sense to throw a
> WARNING and avoid segfault if pgstat machinery is already shutdown? Just
> worried if the code can be reached from multiple paths and testing all of
> those would be difficult for extension developers, especially given this
> may happen in error recovery path.

I'm not sure how extensions can face this problem, but..

pgstat is designed not to lose reported numbers. The assertion is
manifets that intention.  It is not enabled on non-assertion builds
and pgstat enters undefined state then maybe crash after the assertion
point.  On the other hand I don't think we want to perform the same
check for the all places the assertion exists on non-assertion builds.

We cannot simply replace the assertion with ereport().

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center