Date: Tue, 4 Sep 2018 09:51:28 -0300
From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
Cc: Michael Paquier <michael@paquier.xyz>,
	Pg Hackers <pgsql-hackers@postgresql.org>
Subject: Re: pointless check in RelationBuildPartitionDesc
Message-ID: <20180904125128.ntliwrygsylaar4b@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <d72552fa-61c6-c981-e04a-6e5079db0a2c@lab.ntt.co.jp>
User-Agent: NeoMutt/20170306-137-4415bd-dirty (1.8.0)
Precedence: bulk

On 2018-Sep-04, Amit Langote wrote:

> On 2018/09/04 13:08, Alvaro Herrera wrote:

> > I think it'd be pointless noise.  If we really want to protect against
> > that, I think we should promote the Assert for relpartbound's isnull
> > flag into an if test.
> 
> So that we can elog(ERROR, ...) if isnull is true?  If so, I agree,
> because that's what should've been done here anyway (for a value read from
> the disk that is).

Yes.

I wonder now what's the value of using an Assert for this, BTW: if those
are enabled, then we'll crash saying "this column is null and shouldn't!".
If they are disabled, we'll instead crash (possibly in production)
trying to decode the field.  What was achieved?

With an elog(ERROR) the reaction is the expected one: the current
transaction is aborted, but the server continues to run.

> I think we should check relispartition then too.

Well, we don't check every single catalog flag in every possible code
path just to ensure it's sane.  I don't see any reason to do differently
here.  We rely heavily on the catalog contents to be correct, and
install defenses against user-induced corruption that would cause us to
crash; but going much further than that would be excessive IMO.

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