Thread: [HACKERS] brin autosummarization -- autovacuum "work items"

I think one of the most serious issues with BRIN indexes is how they
don't get updated automatically as the table is filled.  This patch
attempts to improve on that.  During brininsert() time, we check whether
we're inserting the first item on the first page in a range.  If we are,
request autovacuum to do a summarization run on that table.  This is
dependent on a new reloption for BRIN called "autosummarize", default
off.

The way the request works is that autovacuum maintains a DSA which can
be filled by backends with "work items".  Currently, work items can
specify a BRIN summarization of some specific index; in the future we
could use this framework to request other kinds of things that do not
fit in the "dead tuples / recently inserted tuples" logic that autovac
currently uses to decide to vacuum/analyze tables.

However, it seems I have not quite gotten the hang of DSA just yet,
because after a couple of iterations, crashes occur.  I think the reason
has to do with either a resource owner clearing the DSA at an unwelcome
time, or perhaps there's a mistake in my handling of DSA "relative
pointers" stuff.

This patch was initially written by Simon Riggs, who envisioned that
brininsert itself would invoke the summarization.  However, this doesn't
work because summarization requires having ShareUpdateExclusive lock,
which brininsert doesn't have.  So I modified things to instead use the
DSA stuff.  (He also set things up so that brininsert would only
summarize the just-filled range, but I didn't preserve that idea in the
autovacuum-based implementation; some changed lines there can probably
be removed.)

-- 
Álvaro Herrera                PostgreSQL Expert, https://www.2ndQuadrant.com/

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, Mar 1, 2017 at 5:58 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> I think one of the most serious issues with BRIN indexes is how they
> don't get updated automatically as the table is filled.  This patch
> attempts to improve on that.  During brininsert() time, we check whether
> we're inserting the first item on the first page in a range.  If we are,
> request autovacuum to do a summarization run on that table.  This is
> dependent on a new reloption for BRIN called "autosummarize", default
> off.

Nice.

> The way the request works is that autovacuum maintains a DSA which can
> be filled by backends with "work items".  Currently, work items can
> specify a BRIN summarization of some specific index; in the future we
> could use this framework to request other kinds of things that do not
> fit in the "dead tuples / recently inserted tuples" logic that autovac
> currently uses to decide to vacuum/analyze tables.
>
> However, it seems I have not quite gotten the hang of DSA just yet,
> because after a couple of iterations, crashes occur.  I think the reason
> has to do with either a resource owner clearing the DSA at an unwelcome
> time, or perhaps there's a mistake in my handling of DSA "relative
> pointers" stuff.

Ok, I'll take a look.  It's set up for ease of use in short lifespan
situations like parallel query, and there are a few extra hoops to
jump through for longer lived DSA areas.

-- 
Thomas Munro
http://www.enterprisedb.com

On Wed, Mar 1, 2017 at 6:06 PM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:
> On Wed, Mar 1, 2017 at 5:58 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
>> I think one of the most serious issues with BRIN indexes is how they
>> don't get updated automatically as the table is filled.  This patch
>> attempts to improve on that.  During brininsert() time, we check whether
>> we're inserting the first item on the first page in a range.  If we are,
>> request autovacuum to do a summarization run on that table.  This is
>> dependent on a new reloption for BRIN called "autosummarize", default
>> off.
>
> Nice.
>
>> The way the request works is that autovacuum maintains a DSA which can
>> be filled by backends with "work items".  Currently, work items can
>> specify a BRIN summarization of some specific index; in the future we
>> could use this framework to request other kinds of things that do not
>> fit in the "dead tuples / recently inserted tuples" logic that autovac
>> currently uses to decide to vacuum/analyze tables.
>>
>> However, it seems I have not quite gotten the hang of DSA just yet,
>> because after a couple of iterations, crashes occur.  I think the reason
>> has to do with either a resource owner clearing the DSA at an unwelcome
>> time, or perhaps there's a mistake in my handling of DSA "relative
>> pointers" stuff.
>
> Ok, I'll take a look.  It's set up for ease of use in short lifespan
> situations like parallel query, and there are a few extra hoops to
> jump through for longer lived DSA areas.

I haven't tested this, but here is some initial feedback after reading
it through once:
/* * Attach to an area given a handle generated (possibly in another process) by
- * dsa_get_area_handle.  The area must have been created with dsa_create (not
+ * dsa_get_handle.  The area must have been created with dsa_create (not * dsa_create_in_place). */

This is an independent slam-dunk typo fix.

+    /*
+     * Set up our DSA so that backends can install work-item requests.  It may
+     * already exist as created by a previous launcher.
+     */
+    if (!AutoVacuumShmem->av_dsa_handle)
+    {
+        LWLockAcquire(AutovacuumLock, LW_EXCLUSIVE);
+        AutoVacuumDSA = dsa_create(LWTRANCHE_AUTOVACUUM);
+        AutoVacuumShmem->av_dsa_handle = dsa_get_handle(AutoVacuumDSA);
+        /* delay array allocation until first request */
+        AutoVacuumShmem->av_workitems = InvalidDsaPointer;
+        LWLockRelease(AutovacuumLock);
+    }
+    else
+        AutoVacuumDSA = dsa_attach(AutoVacuumShmem->av_dsa_handle);

I haven't looked into the autovacuum launcher lifecycle, but if it can
be restarted as implied by the above then I see no reason to believe
that the DSA area still exists at the point where you call
dsa_attach() here.  DSA areas are reference counted, so if there ever
a scenario where no backend is currently attached, then it will be
destroyed and this call will fail.  If you want to create a DSA area
that lasts until cluster shutdown even while all backends are
detached, you need to call dsa_pin() after creating it.

In AutoVacuumRequestWork:

+        AutoVacuumDSA = dsa_attach(AutoVacuumShmem->av_dsa_handle);
+
+        if (!AutoVacuumDSA)
+        {
+            /* cannot attach?  disregard request */
+            LWLockRelease(AutovacuumLock);
+            return;
+        }

dsa_attach either succeeds or throws, so that conditional code is unreachable.

+            /* XXX surely there is a simpler way to do this */
+            wi_ptr = AutoVacuumShmem->av_workitems + sizeof(AutovacWorkItems) +
+                sizeof(AutoVacuumWorkItem) * i;
+            workitem = (AutoVacuumWorkItem *)
dsa_get_address(AutoVacuumDSA, wi_ptr);

It'd probably be simpler to keep hold of the backend-local address of
the the base of the workitems array and then use regular C language
facilities like array notation to work with it: workitems =
&workitems[i], and then:

+    /* ... and put it on autovacuum's to-do list */
+    workitems->avs_usedItems = wi_ptr;

Considering that i is really an index into the contiguous workitems
array, maybe you should really just be storing the index from i here,
instead of dealing in dsa_pointers.  The idea with dsa_pointers is
that they're useful for complex data structures that might point into
different DSM segments, like a hash table or binary tree that has
internal pointers that could pointer arbitrary other objects in a data
structure because it's allocated in incremental pieces.  Here, you are
dealing with objects in a contiguous memory space of fixed size.  This
leads to a bigger question about this design, which I'll ask at the
end.

Then at the bottom of AutoVacuumRequestWork:

+    LWLockRelease(AutovacuumLock);
+    dsa_detach(AutoVacuumDSA);
+    AutoVacuumDSA = NULL;
+}

I'm guessing that you intended to remain attached, rather than
detaching at the end like this?  Otherwise every backend that is
inserting lots of new data attaches and detaches repeatedly, which
seems unnecessary.  If you do that, you'll need to run
dsa_pin_mapping() after attaching, or else the DSA area will be
unmapped at end of transaction and future attempts to access it will
segfault.

In dsm.c:

-    ResourceOwnerEnlargeDSMs(CurrentResourceOwner);
+    if (CurrentResourceOwner)
+        ResourceOwnerEnlargeDSMs(CurrentResourceOwner);

... and then:

-    seg->resowner = CurrentResourceOwner;
-    ResourceOwnerRememberDSM(CurrentResourceOwner, seg);
+    if (CurrentResourceOwner)
+    {
+        seg->resowner = CurrentResourceOwner;
+        ResourceOwnerRememberDSM(CurrentResourceOwner, seg);
+    }

This makes sense.  It allows DSMs (and therefore also DSA areas) to be
created when you don't have a resource owner.  In fact dsm.c
contradicts itself sightly in this area: dsm_create() clearly believes
that seg->segment can be NULL after dsm_create_descriptor() returns
(see code near "too many dynamic shared memory segments" error), but
dsm_create_descriptor() doesn't believe that to be the case without
your patch, so perhaps this should be a separate commit to fix that
rough edge.  However, I think dsm_create_descriptor() still needs to
assign seg->resowner even when it's NULL, otherwise it's
uninitialised.

My solution to this problem when I wrote a couple of different things
that used long lifetime DSA areas (experimental things not posted on
this list) was to define a CurrentResourceOwner with a name like "Foo
Top Level", and then after creating/attaching the segment I'd call
dsa_pin_mapping() which in turn calls dsm_pin_mapping() on all
segments.  Your solution starts out in the pinned mapping state
instead (= disconnected from resource owner machinery), which is
better.

In AutoVacWorkerMain:

+            if (workitem->avw_database == MyDatabaseId &&
!workitem->avw_active)

Stepping over already-active items in the list seems OK because the
number of such items is bounded by the number of workers.  Stepping
over all items for other databases sounds quite expensive if it
happens very often, because these are not so bounded.  Ah, there can't
be more than NUM_WORKITEMS, which is small.

+                /*
+                 * Remove the job we just completed from the used list and put
+                 * the array item back on the free list.
+                 */
+                workitems->avs_usedItems = workitem->avw_next;

Isn't this corrupting the list avs_usedItems queue if avw_next points
to an item that some other worker has removed from the list while we
were working on our item?

Stepping back from the code a bit:

What is your motivation for using DSA?  It seems you are creating an
area and then using it to make exactly one allocation of a constant
size known up front to hold your fixed size workitems array.  You
don't do any dynamic allocation at runtime, apart from the detail that
it happens to allocated on demand.  Perhaps it would make sense if you
had a separate space per database or something like that, so that the
shared memory need would be dynamic?

It looks like outstanding autosummarisation work will be forgotten if
you restart before it is processed.  Over in another thread[1] we
exchanged emails on another way to recognise that summarisation work
needs to be done, if we are only interested in unsummarised ranges at
the end of the heap.  I haven't studied BRIN enough to know if that is
insufficient: can you finish up with unsummarised ranges not in a
contiguous range at the end of the heap?  If so, perhaps the BRIN
index itself should also have a way to record that certain non-final
ranges are unsummarised but should be summarised asynchronously?  Then
the system could be made to behave exactly the same way no matter when
reboots occur, which seems like a good property.

[1] https://www.postgresql.org/message-id/20170130191640.2johoyume5v2dbbq%40alvherre.pgsql

-- 
Thomas Munro
http://www.enterprisedb.com

On Wed, Mar 1, 2017 at 6:06 PM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:
> On Wed, Mar 1, 2017 at 5:58 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
>> I think one of the most serious issues with BRIN indexes is how they
>> don't get updated automatically as the table is filled.  This patch
>> attempts to improve on that.  During brininsert() time, we check whether
>> we're inserting the first item on the first page in a range.  If we are,
>> request autovacuum to do a summarization run on that table.  This is
>> dependent on a new reloption for BRIN called "autosummarize", default
>> off.
>
> Nice.

Another thought about this design:  Why autovacuum?

Obviously we don't want to get to the point where you start up
PostgreSQL and see 25 llines like BRIN SummarizationLauncher started,
Foo Launcher started, Bar Launcher started, ... but perhaps there is a
middle ground between piling all the background work into the
autovacuum framework, and making new dedicated launchers and workers
for each thing.

Is there some way we could turn this kind of maintenance work into a
'task' (insert better word) that can be scheduled to run
asynchronously by magic workers, so that you don't have to supply a
whole worker and main loop and possibly launcher OR jam new
non-vacuum-related work into the vacuum machinery, for each thing like
this that someone decides to invent?

-- 
Thomas Munro
http://www.enterprisedb.com

Thomas Munro wrote:

> Another thought about this design:  Why autovacuum?

One reason is that autovacuum is already there, so it's convenient to
give it the responsibility for this kind of task.  Another reason is
that autovacuum is already doing this, via vacuum.  I don't see the
need to have a completely different process set for tasks that belong to
the system's cleanup process.

With this infrastructure, we could have other types of individual tasks
that could be run by autovacuum.  GIN pending list cleanup, for
instance, or VM bit setting.  Both of those are currently being doing
whenever VACUUM fires, but only because at the time they were written
there was no other convenient place to hook them onto.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Thomas Munro wrote:

> What is your motivation for using DSA?  It seems you are creating an
> area and then using it to make exactly one allocation of a constant
> size known up front to hold your fixed size workitems array.  You
> don't do any dynamic allocation at runtime, apart from the detail that
> it happens to allocated on demand.  Perhaps it would make sense if you
> had a separate space per database or something like that, so that the
> shared memory need would be dynamic?

Well, the number of work items is currently fixed; but if you have many
BRIN indexes, then you'd overflow (lose requests).  By using DSA I am
making it easy to patch this afterwards so that an arbitrary number of
requests can be recorded.  

> It looks like outstanding autosummarisation work will be forgotten if
> you restart before it is processed.

That's true.  However, it would be easy to make index scans also request
work items if they find a full page range that should have been
summarized, so if they are lost, it's not a big deal.

> Over in another thread[1] we
> exchanged emails on another way to recognise that summarisation work
> needs to be done, if we are only interested in unsummarised ranges at
> the end of the heap.  I haven't studied BRIN enough to know if that is
> insufficient: can you finish up with unsummarised ranges not in a
> contiguous range at the end of the heap?

If we include my other patch to remove the index tuple for a certain
range, then yes, it can happen.  (That proposed patch requires manual
action, but range invalidation could also be invoked automatically when,
say, a certain number of tuples are removed from a page range.)

> If so, perhaps the BRIN
> index itself should also have a way to record that certain non-final
> ranges are unsummarised but should be summarised asynchronously?

I think this is unnecessary, and would lead to higher operating
overhead.  With this patch, it's very cheap to file a work item.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Here's a version of this patch which I consider final.

Thanks for your tips on using DSA.  No crashes now.

I am confused about not needing dsa_attach the second time around.  If I
do that, the dsa handle has been 0x7f'd, which I don't understand since
it is supposed to be allocated in TopMemoryContext.  I didn't dig too
deep to try and find what is causing that behavior.  Once we do, it's
easy to remove the dsa_detach/dsa_attach calls.

I added a new SQL-callable function to invoke summarization of an
individual page range.  That is what I wanted to do in vacuum (rather
than a scan of the complete index), and it seems independently useful.

I also removed the behavior that on index creation the final partial
block range is always summarized.  It's pointless.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, Mar 21, 2017 at 4:10 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> Well, the number of work items is currently fixed; but if you have many
> BRIN indexes, then you'd overflow (lose requests).  By using DSA I am
> making it easy to patch this afterwards so that an arbitrary number of
> requests can be recorded.

But that could also use an arbitrarily large amount of memory, and any
leaks will be cluster-lifespan.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Alvaro Herrera wrote:

> I also removed the behavior that on index creation the final partial
> block range is always summarized.  It's pointless.

I just pushed this, without this change, because it breaks
src/test/modules/brin.  I still think it's pointless, but it'd require
more than one line to change.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services