pgsql: Fix handling of all-zero pages in SP-GiST vacuum.
Fix handling of all-zero pages in SP-GiST vacuum.
SP-GiST initialized an all-zeros page at vacuum, but that was not
WAL-logged, which is not safe. You might get a torn page write, when it gets
flushed to disk, and end-up with a half-initialized index page. To fix,
leave it in the all-zeros state, and add it to the FSM. It will be
initialized when reused. Also don't set the page-deleted flag when recycling
an empty page. That was also not WAL-logged, and a torn write of that would
cause the page to have an invalid checksum.
Backpatch to 9.2, where SP-GiST indexes were added.
Branch
------
master
Details
-------
http://git.postgresql.org/pg/commitdiff/023430abf72eb7d335430e241065d5ed19ddd94b
Modified Files
--------------
src/backend/access/spgist/spgvacuum.c | 27 ++++++++-------------------
src/include/access/spgist_private.h | 4 ++--
2 files changed, 10 insertions(+), 21 deletions(-)
--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers
Heikki Linnakangas <heikki.linnakangas@iki.fi> writes:
Fix handling of all-zero pages in SP-GiST vacuum.
I'm a little bit uncomfortable with the way that this patch presumes that
PageIsEmpty (a) is safe on an all-zeroes page and (b) will return true for
such a page. Yes, it does work at the moment; but I don't think we are
making such an assumption anyplace else, and in view of your other two
concurrent patches, it doesn't seem very future-proof here either.
I recommend that the code look more like
if (!SpGistBlockIsRoot(blkno))
{
if (PageIsNew(page) || PageIsEmpty(page))
regards, tom lane
--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers
On 07/27/2015 06:36 PM, Tom Lane wrote:
Heikki Linnakangas <heikki.linnakangas@iki.fi> writes:
Fix handling of all-zero pages in SP-GiST vacuum.
I'm a little bit uncomfortable with the way that this patch presumes that
PageIsEmpty (a) is safe on an all-zeroes page and (b) will return true for
such a page. Yes, it does work at the moment; but I don't think we are
making such an assumption anyplace else, and in view of your other two
concurrent patches, it doesn't seem very future-proof here either.
I recommend that the code look more likeif (!SpGistBlockIsRoot(blkno))
{
if (PageIsNew(page) || PageIsEmpty(page))
I thought I saw other places that assumed that, and I even thought I saw
a comment somewhere documenting that usage. But now that I grep around,
I see no such thing.
I'll go change that...
- Heikki
--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers