From: Tom Lane <tgl@sss.pgh.pa.us>
To: Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>
cc: andres@anarazel.de, michael@paquier.xyz, pryzby@telsasoft.com,
        pgsql-hackers@postgresql.org
Subject: Re: ENOSPC FailedAssertion("!(RefCountErrors == 0)"
In-reply-to: <20180718.124220.08087886.horiguchi.kyotaro@lab.ntt.co.jp>
References: <20436.1531751966@sss.pgh.pa.us> <4993.1531773411@sss.pgh.pa.us>
 <14892.1531872065@sss.pgh.pa.us>
 <20180718.124220.08087886.horiguchi.kyotaro@lab.ntt.co.jp>
Comments: In-reply-to Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>
	message dated "Wed, 18 Jul 2018 12:42:20 +0900"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <10689.1531924381.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable
Date: Wed, 18 Jul 2018 10:33:01 -0400
Message-ID: <10690.1531924381@sss.pgh.pa.us>
Precedence: bulk

Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> writes:
> Hello. I confirmed that this patch fixes the crash.

Thanks for double-checking.

> At Tue, 17 Jul 2018 20:01:05 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote i=
n <14892.1531872065@sss.pgh.pa.us>
>> Further investigation showed that the part of that code that was
>> actually needed was not the creation of a child ResourceOwner, but rath=
er
>> restoration of the old CurrentResourceOwner setting after exiting the
>> logical decoding loop.  Apparently we can come out of that with the
>> TopTransaction resowner being active.

> CurrentResourceOwner doesn't seem to be changed. It is just saved
> during snapshot export and used just as a flag only for checking
> for duplicate snapshot exporting.

I tried to remove "CurrentResourceOwner =3D old_resowner" from
logicalfuncs.c, and got a failure in the contrib/test_decoding test.
I investigated the reason and found that it was because
CurrentResourceOwner was equal to TopTransactionResourceOwner when we
come out of the decoding loop.  If we don't restore it then subsequent
executor activity gets logged in the wrong resowner.

It is true that the one in slotfuncs.c seems to be unnecessary,
but I thought it best to leave it there; it's cheap, and maybe
there's a problem in cases not exercised in the regression tests.

> So couldn't we use TopTransactionResourceOwner instead of
> AuxProcessResrouceOwner? I feel a bit uneasy that bootstrap and
> standalone-backend have *AuxProcess*ResourceOwner.

Since the aux processes aren't running transactions, I didn't think
that TopTransactionResourceOwner was appropriate.  There's also
a problem for bootstrap and standalone backend cases: those do run
transactions and therefore create/destroy TopTransactionResourceOwner,
leaving nothing behind for ShutdownXLOG to use if it tries to use
that.  We need an extra resowner somewhere.

It's true that if you adopt a narrow definition of "aux process"
as being "one that goes through AuxiliaryProcessMain", calling
this extra resowner AuxProcessResourceOwner is a bit of a misnomer.
I couldn't think of something better to call it, though.  With a
slightly wider understanding of "auxiliary process" as being anything
that's not the postmaster or a client-session backend, it's fine.

> It's not about this ptch, but while looking this closer, I found
> the following comment on ShutdownXLOG().
> | /*
> |  * This must be called ONCE during postmaster or standalone-backend sh=
utdown
> |  */
> Is the "postmaster" typo of "bootstrap process"? It is also
> called from checkpointer when non-standlone mode.

Well, it's actually executed by the checkpointer these days, but
that's an implementation detail.  I think this comment is fine:
at some point while the postmaster is shutting down, this should
be done, and done just once.

			regards, tom lane