Re: GiST VACUUM

From: Heikki Linnakangas <hlinnaka(at)iki(dot)fi>
To: Andrey Borodin <x4mmm(at)yandex-team(dot)ru>
Cc: Michael Paquier <michael(at)paquier(dot)xyz>, Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>, Костя Кузнецов <chapaev28(at)ya(dot)ru>
Subject: Re: GiST VACUUM
Date: 2019-03-11 15:03:54
Message-ID: 7e3f29b6-de18-6bd5-ab06-db2eaa180173@iki.fi
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

On 10/03/2019 18:40, Andrey Borodin wrote:
> Here's new version of the patch for actual page deletion.
> Changes:
>
> 1. Fixed possible concurrency issue:
>
> We were locking child page while holding lock on internal page. Notes
> in GiST README recommend locking child before parent. Thus now we
> unlock internal page before locking children for deletion, and lock
> it again, check that everything is still on it's place and delete
> only then.

Good catch. The implementation is a bit broken, though. This segfaults:

create table gist_point_tbl(id int4, p point);
create index gist_pointidx on gist_point_tbl using gist(p);

insert into gist_point_tbl (id, p)
select g, point((1+g) % 100, (1+g) % 100) from generate_series(1,
10000) g;
insert into gist_point_tbl (id, p)
select -g, point(1000, 1000) from generate_series(1, 10000) g;

delete from gist_point_tbl where id >= 0;
vacuum verbose gist_point_tbl;

BTW, we don't seem to have test coverage for this in the regression
tests. The tests that delete some rows, where you updated the comment,
doesn't actually seem to produce any empty pages that could be deleted.

> One thing still bothers me. Let's assume that we have internal page
> with 2 deletable leaves. We lock these leaves in order of items on
> internal page. Is it possible that 2nd page have follow-right link on
> 1st and someone will lock 2nd page, try to lock 1st and deadlock with
> VACUUM?

Hmm. If the follow-right flag is set on a page, it means that its right
sibling doesn't have a downlink in the parent yet. Nevertheless, I think
I'd sleep better, if we acquired the locks in left-to-right order, to be
safe.

An easy cop-out would be to use LWLockConditionalAcquire, and bail out
if we can't get the lock. But if it's not too complicated, it'd be nice
to acquire the locks in the correct order to begin with.

I did a round of cleanup and moving things around, before I bumped into
the above issue. I'm including them here as separate patches, for easier
review, but they should of course be squashed into yours. For
convenience, I'm including your patches here as well, so that all the
patches you need are in the same place, but they are identical to what
you posted.

- Heikki

Attachment Content-Type Size
0001-Add-block-set-data-structure.patch text/x-patch 16.1 KB
0002-Delete-pages-during-GiST-VACUUM.patch text/x-patch 20.2 KB
0003-Minor-cleanup.patch text/x-patch 8.7 KB
0004-Move-the-page-deletion-logic-to-separate-function.patch text/x-patch 18.8 KB
0005-Remove-numEmptyPages-.-it-s-not-really-needed.patch text/x-patch 2.5 KB
0006-Misc-cleanup.patch text/x-patch 3.4 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2019-03-11 15:06:33 Re: Portability of strtod (was Re: pgsql: Include GUC's unit, if it has one, in out-of-range error message)
Previous Message Tom Lane 2019-03-11 14:46:12 Re: Oddity with parallel safety test for scan/join target in grouping_planner