Date: Mon, 11 May 2020 19:41:55 -0500
From: Justin Pryzby <pryzby@telsasoft.com>
To: Amit Kapila <amit.kapila16@gmail.com>
Cc: Bruce Momjian <bruce@momjian.us>,
	PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: PG 13 release notes, first draft
Message-ID: <20200512004155.GP18456@telsasoft.com>
References: <20200505031600.GB21752@momjian.us>
 <CAA4eK1+tOd6vTXBjUyjEQF+NALdoX8d-EZu5HEs_nxZ-huX2Hw@mail.gmail.com>
 <CAA4eK1+DDmycMNy9VQH8dFcPnkF5j8MNWVHL4yWNZ_0BL8HF2Q@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CAA4eK1+DDmycMNy9VQH8dFcPnkF5j8MNWVHL4yWNZ_0BL8HF2Q@mail.gmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

On Mon, May 11, 2020 at 10:52:41AM +0530, Amit Kapila wrote:
> > 1.  We have allowed an (auto)vacuum to display additional information
> > about heap or index in case of an error in commit b61d161c14 [1].
> > Now, in general, it might not be worth saying much about error
> > information but I think this one could help users in case they have
> > some corruption.  For example, if one of the indexes on a relation has
> > some corrupted data (due to bad hardware or some bug), it will let the
> > user know the index information, and the user can take appropriate
> > action like either Reindex or maybe drop and recreate the index to
> > overcome the problem.

I'm not opposed to including it, but I think it's still true that the user
doesn't need to know in advance that the error message will be additionally
helpful in the event of corruption.  If we were to include more "error" items,
we might also include these:

71a8a4f6e36547bb060dbcc961ea9b57420f7190 Add backtrace support for error reporting
17a28b03645e27d73bf69a95d7569b61e58f06eb Improve the internal implementation of ereport().
05f18c6b6b6e4b44302ee20a042cedc664532aa2 Added relation name in error messages for constraint checks.
33753ac9d7bc83dd9ccee9d5e678ed78a0725b4e Add object names to partition integrity violations.

> One more observation:
> 
> Allow inserts to trigger autovacuum activity (Laurenz Albe, Darafei
> Praliaskouski)
> This new behavior allows pages to be set as all-visible, which then
> allows index-only scans, ...

> The above sentence sounds to mean that this feature allows index-only
> scans in more number of cases after this feature.  Is that what you
> intend to say? If so, is that correct?  Because I think this will
> allow index-only scans to skip "Heap Fetches" in more cases.

I think what you mean is that the autovacuum feature, in addition to
encouraging the *planner* to choose an indexonly scan, will *also* allow (at
execution time) fewer heap fetches for a plan which would have
already/previously used IOS.  Right ?  So maybe it should say "allows OR
IMPROVES index-only scans" or "allows plans which use IOS to run more
efficiently".

Separate from Amit's comment, I suggest to say:
| This new behavior allows autovacuum to set pages as all-visible, which then
| allows index-only scans, ...

..otherwise it sounds like this feature implemented the concept of
"all-visible".

-- 
Justin