pgsql: Close some holes in BRIN page assignment
Close some holes in BRIN page assignment
In some corner cases, it is possible for the BRIN index relation to be
extended by brin_getinsertbuffer but the new page not be used
immediately for anything by its callers; when this happens, the page is
initialized and the FSM is updated (by brin_getinsertbuffer) with the
info about that page, but these actions are not WAL-logged. A later
index insert/update can use the page, but since the page is already
initialized, the initialization itself is not WAL-logged then either.
Replay of this sequence of events causes recovery to fail altogether.
There is a related corner case within brin_getinsertbuffer itself, in
which we extend the relation to put a new index tuple there, but later
find out that we cannot do so, and do not return the buffer; the page
obtained from extension is not even initialized. The resulting page is
lost forever.
To fix, shuffle the code so that initialization is not the
responsibility of brin_getinsertbuffer anymore, in normal cases;
instead, the initialization is done by its callers (brin_doinsert and
brin_doupdate) once they're certain that the page is going to be used.
When either those functions determine that the new page cannot be used,
before bailing out they initialize the page as an empty regular page,
enter it in FSM and WAL-log all this. This way, the page is usable for
future index insertions, and WAL replay doesn't find trying to insert
tuples in pages whose initialization didn't make it to the WAL. The
same strategy is used in brin_getinsertbuffer when it cannot return the
new page.
Additionally, add a new step to vacuuming so that all pages of the index
are scanned; whenever an uninitialized page is found, it is initialized
as empty and WAL-logged. This closes the hole that the relation is
extended but the system crashes before anything is WAL-logged about it.
We also take this opportunity to update the FSM, in case it has gotten
out of date.
Thanks to Heikki Linnakangas for finding the problem that kicked some
additional analysis of BRIN page assignment code.
Backpatch to 9.5, where BRIN was introduced.
Discussion: /messages/by-id/20150723204810.GY5596@postgresql.org
Branch
------
REL9_5_STABLE
Details
-------
http://git.postgresql.org/pg/commitdiff/fc0a6402306db3cc93a5f760acabab687998be1d
Modified Files
--------------
src/backend/access/brin/brin.c | 43 ++++++
src/backend/access/brin/brin_pageops.c | 234 +++++++++++++++++++++++++++++---
src/include/access/brin_page.h | 1 +
src/include/access/brin_pageops.h | 2 +
4 files changed, 258 insertions(+), 22 deletions(-)
--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers
Alvaro Herrera wrote:
Close some holes in BRIN page assignment
buildfarm evidently didn't like this one :-(
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2015-08-12 16:08:08 -0300, Alvaro Herrera wrote:
Alvaro Herrera wrote:
Close some holes in BRIN page assignment
buildfarm evidently didn't like this one :-(
clang seems to see a (the?) problem:
/home/andres/src/postgresql/src/backend/access/brin/brin_pageops.c:357:6: warning: variable 'extended' is used uninitialized whenever 'if'
condition is false [-Wsometimes-uninitialized]
if (!BufferIsValid(*buffer))
^~~~~~~~~~~~~~~~~~~~~~~
/home/andres/src/postgresql/src/backend/access/brin/brin_pageops.c:371:6: note: uninitialized use occurs here
if (extended)
^~~~~~~~
/home/andres/src/postgresql/src/backend/access/brin/brin_pageops.c:357:2: note: remove the 'if' if its condition is always true
if (!BufferIsValid(*buffer))
^~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/andres/src/postgresql/src/backend/access/brin/brin_pageops.c:330:16: note: initialize the variable 'extended' to silence this warning
bool extended;
Looks to me like it's right. That also explains why it's failing on a
fairly random selection of platforms and compilers...
Andres
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Andres Freund wrote:
On 2015-08-12 16:08:08 -0300, Alvaro Herrera wrote:
Alvaro Herrera wrote:
Close some holes in BRIN page assignment
buildfarm evidently didn't like this one :-(
clang seems to see a (the?) problem:
Ahh, right. There's an identical problem in the other function
(brin_doupdate); maybe the conditional there is more complicated than it
wants to analyze. I was trying randomized memory and wasn't finding
anything ...
BTW I looked around the buildfarm and couldn't find a single member that
displayed these warnings :-(
Looks to me like it's right. That also explains why it's failing on a
fairly random selection of platforms and compilers...
Yeah ...
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2015-08-13 00:24:29 -0300, Alvaro Herrera wrote:
Andres Freund wrote:
On 2015-08-12 16:08:08 -0300, Alvaro Herrera wrote:
Alvaro Herrera wrote:
Close some holes in BRIN page assignment
buildfarm evidently didn't like this one :-(
clang seems to see a (the?) problem:
Ahh, right. There's an identical problem in the other function
(brin_doupdate); maybe the conditional there is more complicated than it
wants to analyze. I was trying randomized memory and wasn't finding
anything ...BTW I looked around the buildfarm and couldn't find a single member that
displayed these warnings :-(
I tripped on clang 3.7 (unreleased) and a fairly extensive set of
warning flags (-Weverything + -Wno-... of the stupid ones)...
FWIW, this very likely would have tripped in valgrind. Just running that
single test will not even be too slow.
Regards,
Andres
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers