Re: 9.5 release notes - Mailing list pgsql-hackers

On Fri, Jun 19, 2015 at 08:21:19PM +0200, Andres Freund wrote:
> Hi,
> 
> On 2015-06-11 00:15:21 -0400, Bruce Momjian wrote:
> > I have committed the first draft of the 9.5 release notes.  You can view
> > the output here:
> 
> So, I did a pass through master's state:
> 
> >      <listitem>
> >       <para>
> >        Add <link linkend="BRIN">Block Range Indexes</> (<acronym>BRIN</>)
> >        (Álvaro Herrera, Heikki Linnakangas, Emre Hasegeli)
> >       </para>
> >
> >       <para>
> >        <acronym>BRIN</> indexes are very compact and store the min/max
> >        values for a range of heap blocks.
> >       </para>
> >      </listitem>
> 
> Maybe we should mention that they're cheap to maintain?

I saw how you were able to get that idea into the release notes with
little additional text --- nice.  :-)  I couldn't figure out how to do
that.

> >      <listitem>
> >       <para>
> >        Improve in-memory hash performance (Tomas Vondra, Robert Haas)
> >       </para>
> >      </listitem>
> 
> "hash performance" is pretty general, there's lot of places where we use
> hashing that aren't affected.

Well, how many of our users even know what a hash is, or when we use
hashing internally, and care where the speedup is, or will understand
it?

The guiding principal is that these release notes are written for our
users, not for us.  I don't see any way to give more details here
without being confusing or overly verbose.

> >      <listitem>
> >       <para>
> >        Improve concurrency of <link linkend="guc-shared-buffers">shared
> >        buffer</> replacement (Robert Haas, Amit Kapila)
> >       </para>
> >      </listitem>
> 
> I think in the end that patch was enhanced to a significant degree by
> making it lockless in d72731a70450b. I think the three (?) involved
> patches should just combined under one entry.

Did you address this already in
c0b0501925eacbf2d9c10cd231bf8a14e7c9ef4c?  I can't tell.

> >      <listitem>
> >       <para>
> >        Improve concurrent locking and buffer scan performance (Andres
> >        Freund, Kevin Grittner)
> >       </para>
> >      </listitem>
> 
> If this is ab5194e6f, I don't think it makes sense to mention "buffer
> scan" - it's just any lwlock, and buffer locks aren't the primary
> benefit (ProcArrayLock, buffer mapping lock probably are that). I also
> don't think Kevin was involved?
> 
> I think ed127002d8 and 4b4b680c should be mentioned in this section as
> well. 4b4b680c will considerably reduce the per backend memory usage for
> servers with large shared buffers.

I think you addressed this, right?

> >    <sect4>
> >     <title>Server Settings</title>
> >
> >     <itemizedlist>
> >
> >      <listitem>
> >       <para>
> >        Replace <varname>checkpoint_segments</> with <link
> >        linkend="guc-min-wal-size"><varname>min_wal_size</></> and
> >        <link linkend="guc-max-wal-size"><varname>max_wal_size</></>
> >        (Heikki Linnakangas)
> >       </para>
> >
> >       <para>
> >        This allows the allocation of a large number of <acronym>WAL</>
> >        files without keeping them if they are not needed.
> >       </para>
> >      </listitem>
> 
> Hm. This affects performance significantly, should we also list it there?

Uh, I saw this as more of a configuration improvement in that you don't
need to waste lots of WAL files if you don't need them.

> >      <listitem>
> >       <para>
> >        Add <acronym>GUC</> <link
> >        linkend="guc-wal-compression"><varname>wal_compression</></> to
> >        enable compression of full page images stored in <acronym>WAL</>
> >        (Rahila Syed, Michael Paquier)
> >       </para>
> >      </listitem>
> 
> Also rather performance relevant?

Well, it is both, but we already have a configuration section, so it
seems most logical there.

> >      <listitem>
> >       <para>
> >        Archive <acronym>WAL</> files with suffix <literal>.partial</>
> >        during standby promotion (Heikki Linnakangas)
> >       </para>
> >      </listitem>
> 
> This should be expanded, will mention to Heikki. Possibly also need to
> be mentioned in the backward incompat section.

Uh, it seemed obscure enough to not mention it as a backward
incompatibility.  I am a little afraid of briefkly trying to explain why
we did it.

> >      <listitem>
> >       <para>
> >        Allow the <link linkend="pg-replication-origin-create">labeling</>
> >        of the origin of logical replication changes (Andres Freund)
> >       </para>
> >
> >       <para>
> >        This helps with change tracking.
> >       </para>
> >      </listitem>
> 
> I think it should be 'origin and progress'. The explanation should
> probably rather be 'This is helpful when implementing replication
> solutions" or something like it.

Done.

> >      <listitem>
> >       <para>
> >        Allow control of table <acronym>WAL</> logging after table creation
> >        with <link linkend="SQL-ALTERTABLE"><command>ALTER TABLE .. SET
> >        LOGGED / UNLOGGED</></> (Fabrízio de Royes Mello)
> >       </para>
> >      </listitem>
> 
> This sounds a bit confusing. Maybe "Allow to convert a WAL logged table
> to an UNLOGGED one, and the other way round"?

I saw you used you wording, but I changed it to this:
       Allow changing of the <acronym>WAL</acronym>       logging status of a table ater creation with <link
linkend="SQL-ALTERTABLE"><command>ALTERTABLE .. SET LOGGED /       UNLOGGED</></> (Fabrízio de Royes Mello)
 

> >    <sect4>
> >     <title>System Information Functions and Views</title>
> 
> I wonder if
> 
> >      <listitem>
> >       <para>
> >        Report the backends holding replication slots in <link
> >        linkend="catalog-pg-replication-slots"><structname>pg_replication_slots</></>
> >        (Craig Ringer)
> >       </para>
> >
> >       <para>
> >        The new output column is <structname>active_pid</>.
> >       </para>
> >      </listitem>
> 
> shouldn't be moved her?

OK, makes sense, I guess, though it fits in two categories so it is
unclear exactly which one is best.

> >      <listitem>
> >       <para>
> >        Allow <application>pg_dump</> to share a snapshot taken by another
> >        session using <option>--snapshot</> (Simon Riggs, Michael Paquier)
> >       </para>
> >
> >       <para>
> >        The remote snapshot must have been exported by
> >        <function>pg_export_snapshot()</> or been defined when creating
> >        a logical replication slot.
> 
> 'or exported by logical replication slot creation'?

OK, I used that now.

> >        This can be used by parallel
> >        <application>pg_dump</> to use a consistent snapshot across
> >        <application>pg_dump</> processes.
> >       </para>
> 
> What do you mean by this comment? Parallel pg_dump internally does all
> the snapshot stuff already, and it's independent of this option.

Sorry, parallel was unclear here. I was referring to another pg_dump
running in parallel, not pg_dump -j.  Updated text:
This can be used by another <application>pg_dump</> to use a sharea consistent snapshot across <application>pg_dump</>
processes.

> >      <listitem>
> >       <para>
> >        Change <application>pg_ctl</> default shutdown mode from
> >        <literal>smart</> to <literal>fast</> (Bruce Momjian)
> >       </para>
> >      </listitem>
> 
> Wonder if this should be listed as an incompatibility. This does have
> have impact on existing setups/scripts.

Oh, yes! This is a big incompatibility --- sorry I missed that ---
moved.

> >      <listitem>
> >       <para>
> >        Add basic atomics <acronym>API</> support (Andres Freund, Oskari
> >        Saarenmaa)
> >       </para>
> >      </listitem>
> 
> Out of fairness I think either Oskari shouldn't be listed, or Amit
> should as well. Amit surely has spent more time on the patch than
> Oskari.

I see you fixed this --- good.

> >      <listitem>
> >       <para>
> >        Add native compiler and memory barriers for <productname>Solaris
> >        Studio</> (Oskari Saarenmaa)
> >       </para>
> >
> >       <para>
> >        IS THIS PART OF ATOMICS?
> >       </para>
> >      </listitem>
> 
> Not really, barriers are independent. But I guess we could just combine
> it nonetheless.

I see you took care of this.

> >   <sect3>
> >    <title>Additional Modules</title>
> >
> >    <itemizedlist>
> >
> >     <listitem>
> >      <para>
> >       Add <link linkend="app-pgrewind"><application>pg_rewind</></>,
> >       which allows re-synchronizing a master server after failback
> >       (Heikki Linnakangas)
> >      </para>
> >     </listitem>
> 
> Hm. pg_rewind isn't in contrib, so is this the right place for this?

Tom moved it in this commit, 85c25fdbd7d624928bb8a1f1fd1e5043ec35dd74.
> I think 647248e3708, 4fe384bd85, 4f85fde8, 59f71a0d0 should also be

I couldn't look up 647248e3708, I got "unknown revision or path not in
the working tree."

For 4fe384bd85:
Process 'die' interrupts while reading/writing from the clientsocket.

I am unclear if this is something to mention, or just a rare event that
we now avoid.

For 4f85fde8:
Introduce and use infrastructure for interrupt processing duringclient reads.

I had the same feeling.  The same with 59f71a0d0:
Add a default local latch for use in signal handlers.

These feel very internal to me.

> mentioned. They're amongst the most invasive changes in 9.5, and they do
> have implications for developers and users.

Again, the question is whether it is general enough to inform our user
community about.

> f5ae3ba4/779fdcd should be listed as improving bitmap index scan
> performance.

That is these two:
Make tbm_add_tuples more efficient by caching the last acccessed page.Improve new caching logic in tbm_add_tuples().

Again, as mentioned above, are these of enough general interest?

I am sure I have made some mistakes above, so please let me know what
else I need to do.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + Everyone has their own god. +