Date: Wed, 3 Jun 2020 17:44:48 -0400
From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>,
	Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
	sitnikov.vladimir@gmail.com,
	PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: SIGSEGV from START_REPLICATION 0/XXXXXXX in XLogSendPhysical ()
 at walsender.c:2762
Message-ID: <20200603214448.GA901@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20200602042456.GA3594@paquier.xyz>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

On 2020-Jun-02, Michael Paquier wrote:

> I can note as well that StartLogicalReplication() moves in this sense
> by setting xlogreader to be the one from logical_decoding_ctx once the
> decoding context has been created.
> 
> This results in the attached.  The extra test from upthread to check
> that logical decoding is not allowed in a non-database WAL sender is a
> good idea, so I have kept it.

I don't particularly disagree with your proposed patch -- in fact, it
seems to make things cleaner.  It is a little wasteful, but I don't
really mind that.  It's just some memory, and it's not a significant
amount.

That said, I would *also* apply Kyotaro's proposed patch to prohibit a
physical standby running with a logical slot, if only because that
reduces the number of combinations that we need to test and keep our
collective heads straight about.  Just reject the weird case and have
one type of slot for each type of replication.  I didn't even think this
was at all possible.

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