Horton

Thread

Commits

  1. Accept slightly-filled pages for tuples larger than fillfactor.

  1. non-HOT update not looking at FSM for large tuple update

    Floris Van Nee <florisvannee@optiver.com> — 2021-02-24T14:44:30Z

    Hi hackers,

Recently we found a table that was slowly, but consistently increasing in size. The table has a low fill-factor set and was updated very frequently. As expected, almost all updates are HOT updates, but for some of the non-HOT updates it always wanted to use a new page, rather than reuse an existing empty page. This led to a steady growth in table size (and a steady growth in the number of empty pages in the table).

I've managed to create a very simple reproducing example that shows the problem (original problem occurred on 12.4, but I've tested this example on latest master). It only occurs for updates where the new tuple is larger than the size of what "fillfactor" would normally allow. In real life, this would only be a very small portion of the updates to a certain table of course, but in this example every update will be this large.

Create a table with a low fill-factor and insert one row into it. Note that, in this case, the row that we're inserting is by itself larger than the "max fill factor space".

create table t1 (a int primary key, b text) with (fillfactor=10);
insert into t1 select 1, (select string_agg('1', '') from generate_series(1,1000)); -- 1000 byte text field
vacuum t1;

postgres=# select * from pg_freespace('t1');
blkno | avail
-------+-------
     0 |  7104
(1 row)

This looks alright - there's 1 page and the available space is indeed roughly 1000 bytes less, because of our tuple and page header.

Now, in a different backend, initiate a longer query.

select pg_sleep(600); -- just sleep 600 seconds so that we have enough time to do some updates during this

Then, in the original backend, update the tuple 7 times.

-- execute this 7 times
update t1 set b=(select string_agg((random()*9)::int::text, '') from generate_series(1,1000)) where a=1;

Cancel the pg_sleep call.
Then execute

vacuum t1; -- cleans rows and updates the fsm

postgres=# select * from pg_freespace('t1');
blkno | avail
-------+-------
     0 |  8128
     1 |  7104
(2 rows)

This still looks OK. There's an extra page, because a total of 8 tuples needed to kept alive for the pg_sleep query. These didn't fit on one page, so a new page was created.

Now, repeat it (the pg_sleep, update 7 times, cancel the pg_sleep and vacuum).

postgres=# select * from pg_freespace('t1');
blkno | avail
-------+-------
     0 |  8128
     1 |  8128
     2 |  7104
(3 rows)

This does not look good anymore. The tuple was on page 1, so at first there were several HOT updates on page 1. Then, when page 1 was full, it needed to search for another page to put the tuple. It did not consider page 0, but instead decided to create a new page 2.

Repeating this process would create a new page each time, never reusing the empty old pages.

The reason it does not consider page 0 is because of this piece of code in function RelationGetBufferForTuple in hio.c:

     /* Compute desired extra freespace due to fillfactor option */
     saveFreeSpace = RelationGetTargetPageFreeSpace(relation, HEAP_DEFAULT_FILLFACTOR);
...
     if (len + saveFreeSpace > MaxHeapTupleSize)
     {
           /* can't fit, don't bother asking FSM */
           targetBlock = InvalidBlockNumber;
           use_fsm = false;
     }

The problem here is two-folded: for any non-HOT update of a tuple that's larger than the size of the fillfactor, the fsm will not be used, but instead a new page will be chosen.
This seems to rely on the false assumption that every existing page has at last one tuple on it.
Secondly, and this is a bit trickier.. Even if we would ask the FSM to come up with a free page with a free size of "MaxHeapTupleSize", it wouldn't find anything... This is, because the FSM tracks free space excluding any unused line pointers. In this example, if we look at block 0:

postgres=# select * from page_header(get_raw_page('t1', 0));
    lsn    | checksum | flags | lower | upper | special | pagesize | version | prune_xid
-----------+----------+-------+-------+-------+---------+----------+---------+-----------
0/16D75A0 |        0 |     5 |    52 |  8192 |    8192 |     8192 |       4 |         0
(1 row)

postgres=# select * from heap_page_items(get_raw_page('t1', 0));
lp | lp_off | lp_flags | lp_len | t_xmin | t_xmax | t_field3 | t_ctid | t_infomask2 | t_infomask | t_hoff | t_bits | t_oid | t_data
----+--------+----------+--------+--------+--------+----------+--------+-------------+------------+--------+--------+-------+--------
  1 |      0 |        0 |      0 |        |        |          |        |             |            |        |        |       |
  2 |      0 |        0 |      0 |        |        |          |        |             |            |        |        |       |
  3 |      0 |        0 |      0 |        |        |          |        |             |            |        |        |       |
  4 |      0 |        0 |      0 |        |        |          |        |             |            |        |        |       |
  5 |      0 |        0 |      0 |        |        |          |        |             |            |        |        |       |
  6 |      0 |        0 |      0 |        |        |          |        |             |            |        |        |       |
  7 |      0 |        0 |      0 |        |        |          |        |             |            |        |        |       |
(7 rows)

There are 7 line pointers on this page, consuming 28 bytes. Plus the 24 byte header, that means that lower=52. However, all line pointers are unused, so the page really is empty. The FSM does not see the page as empty though, as it only looks at "upper-lower".

When asking the FSM for slightly less space (MaxHeapTupleSize - 50 for example), it does find the free pages. I've confirmed that with such a hack the table is not growing indefinitely anymore. However, this number 50 is rather arbitrary obviously, as it depends on the number of unused line items on a page, so that's not a proper way to fix things.

In any case, the behavior feels like a bug to me, but I don't know what the best way would be to fix it. Thoughts?

-Floris

  2. Re: non-HOT update not looking at FSM for large tuple update

    John Naylor <john.naylor@enterprisedb.com> — 2021-02-24T19:35:18Z

    On Wed, Feb 24, 2021 at 10:44 AM Floris Van Nee <florisvannee@optiver.com>
wrote:
>
> The problem here is two-folded: for any non-HOT update of a tuple that’s
larger than the size of the fillfactor, the fsm will not be used, but
instead a new page will be chosen.

I confirmed this not only non-HOT updates, but regular inserts, which are
the same thing in this context.

> This seems to rely on the false assumption that every existing page has
at last one tuple on it.

Yep.

> Secondly, and this is a bit trickier.. Even if we would ask the FSM to
come up with a free page with a free size of “MaxHeapTupleSize”, it
wouldn’t find anything… This is, because the FSM tracks free space
excluding any unused line pointers.

> There are 7 line pointers on this page, consuming 28 bytes. Plus the 24
byte header, that means that lower=52. However, all line pointers are
unused, so the page really is empty. The FSM does not see the page as empty
though, as it only looks at “upper-lower”.
>
>
>
> When asking the FSM for slightly less space (MaxHeapTupleSize – 50 for
example), it does find the free pages. I’ve confirmed that with such a hack
the table is not growing indefinitely anymore. However, this number 50 is
rather arbitrary obviously, as it depends on the number of unused line
items on a page, so that’s not a proper way to fix things.
>
>
>
> In any case, the behavior feels like a bug to me, but I don’t know what
the best way would be to fix it. Thoughts?

One idea is to take your -50 idea and make it more general and safe, by
scaling the fudge factor based on fillfactor, such that if fillfactor is
less than 100, the requested freespace is a bit smaller than the max. It's
still a bit of a hack, though. I've attached a draft of this idea.

--
John Naylor
EDB: http://www.enterprisedb.com

  3. RE: non-HOT update not looking at FSM for large tuple update

    Floris Van Nee <florisvannee@optiver.com> — 2021-02-24T20:10:23Z

    Hi John,



> One idea is to take your -50 idea and make it more general and safe, by scaling the fudge factor based on fillfactor, such that if fillfactor is less than 100, the requested freespace is a bit smaller than the max. It's still a bit of a hack, though. I've attached a draft of this idea.



You’re right, that’d work better. Though, one thing I'd forgot to mention earlier is that in the "wild" where this occurred, the UPDATEs with these large tuple sizes are much rarer than UPDATEs with a much smaller tuple size. So this means that in reality, when this happens, the empty pages contain more unused line pointers and we’d need to subtract more bytes in order to find those pages in the fsm.



This is the (partial) output of pg_freespace function, bucketed by free space, for a real-life table with fillfactor=10 under the mixed load that I've described.

│ free │   count │

│ 7750 │    2003 │

│ 7800 │    7113 │

│ 7850 │    1781 │

│ 7900 │    6803 │

│ 7950 │   13643 │

│ 8000 │   64779 │

│ 8050 │ 2469665 │

│ 8100 │   61869 │

└──────┴─────────┘

(163 rows)



The ‘free’ column is the bucket where the value is the lower limit. So, free=7500 means between 7500-7549 bytes free, and count is the number of pages that have this amount free according to the fsm.

In this case, the vast majority has between 8050-8099 bytes free according to the FSM. That means that, for this particular case, for a fillfactor of 10, we’d need to subtract ~120 bytes or so in order to properly recycle the pages.



-Floris

  4. RE: non-HOT update not looking at FSM for large tuple update

    Floris Van Nee <florisvannee@optiver.com> — 2021-02-24T20:52:35Z

    > In this case, the vast majority has between 8050-8099 bytes free according to the FSM. That means that, for this particular case, for a fillfactor of 10, we’d need to subtract ~120 bytes or so in order to properly recycle the pages.

Also, I think this "fudge" factor would need to be defined as a percentage of the page size as well. 100 bytes on an 8kB page is quite different than 100 bytes on a 1kB page (although I have no idea if people ever actually compile PG with a different page size, but it is supposed to be supported?).

I also understand the temptation to define it based on the relation's fill factor, as you did in the patch. However, upon some further thought I wonder if that's useful? A relation with a higher fill factor will have a lower 'saveFreeSpace' variable, so it's less likely to run into issues in finding a fresh page, except if the tuple you're inserting/updating is even larger. However, if that case happens, you'll still be wanting to look for a page that's completely empty (except for the line items). So the only proper metric is 'how many unused line items do we expect on empty pages' and the fillfactor doesn't say much about this. Since this is probably difficult to estimate at all, we may be better off just defining it off MaxHeapTupleSize completely?
For example, we expect 1.5% of the page could be line items, then:

targetFreeSpace = MaxHeapTupleSize * 0.985

-Floris

  5. Re: non-HOT update not looking at FSM for large tuple update

    John Naylor <john.naylor@enterprisedb.com> — 2021-02-24T21:19:47Z

    On Wed, Feb 24, 2021 at 4:52 PM Floris Van Nee <florisvannee@optiver.com>
wrote:

> I also understand the temptation to define it based on the relation's
fill factor, as you did in the patch. However, upon some further thought I
wonder if that's useful? A relation with a higher fill factor will have a
lower 'saveFreeSpace' variable, so it's less likely to run into issues in
finding a fresh page, except if the tuple you're inserting/updating is even
larger. However, if that case happens, you'll still be wanting to look for
a page that's completely empty (except for the line items). So the only
proper metric is 'how many unused line items do we expect on empty pages'
and the fillfactor doesn't say much about this. Since this is probably
difficult to estimate at all, we may be better off just defining it off
MaxHeapTupleSize completely?
> For example, we expect 1.5% of the page could be line items, then:
>
> targetFreeSpace = MaxHeapTupleSize * 0.985

That makes sense, although the exact number seems precisely tailored to
your use case. 2% gives 164 bytes of slack and doesn't seem too large.
Updated patch attached.

--
John Naylor
EDB: http://www.enterprisedb.com

  6. RE: non-HOT update not looking at FSM for large tuple update

    Floris Van Nee <florisvannee@optiver.com> — 2021-02-24T22:29:21Z

    > That makes sense, although the exact number seems precisely tailored to your use case. 2% gives 164 bytes of slack and doesn't seem too large. Updated patch attached.

Yeah, I tried picking it as conservative as I could, but 2% is obviously great too. :-) I can't think of any large drawbacks either of having a slightly larger value.
Thanks for posting the patch!

-Floris

  7. Re: non-HOT update not looking at FSM for large tuple update

    John Naylor <john.naylor@enterprisedb.com> — 2021-02-26T11:14:18Z

    On Wed, Feb 24, 2021 at 6:29 PM Floris Van Nee <florisvannee@optiver.com>
wrote:
>
>
> > That makes sense, although the exact number seems precisely tailored to
your use case. 2% gives 164 bytes of slack and doesn't seem too large.
Updated patch attached.
>
> Yeah, I tried picking it as conservative as I could, but 2% is obviously
great too. :-) I can't think of any large drawbacks either of having a
slightly larger value.
> Thanks for posting the patch!

I've added this to the commitfest as a bug fix and added you as an author.

--
John Naylor
EDB: http://www.enterprisedb.com

  8. RE: non-HOT update not looking at FSM for large tuple update

    Floris Van Nee <florisvannee@optiver.com> — 2021-03-08T15:25:11Z

    > I've added this to the commitfest as a bug fix and added you as an author.

Thanks. Patch looks good to me, but I guess there needs to be someone else reviewing too?
Also, would this be a backpatchable bugfix?

-Floris

  9. Re: non-HOT update not looking at FSM for large tuple update

    Matthias van de Meent <boekewurm+postgres@gmail.com> — 2021-03-08T23:14:19Z

    On Mon, 8 Mar 2021 at 16:25, Floris Van Nee <florisvannee@optiver.com> wrote:
>
> > I've added this to the commitfest as a bug fix and added you as an author.
>
> Thanks. Patch looks good to me, but I guess there needs to be someone else reviewing too?
> Also, would this be a backpatchable bugfix?
>
> -Floris
>

This patch fails to consider that len may be bigger than
MaxHeapTupleSize * 0.98, which in this case triggers a reproducable
PANIC:

=# CREATE TABLE t_failure (a int, b text) WITH (fillfactor = 10); --
force the new FSM calculation for large tuples
CREATE TABLE
=# ALTER TABLE t_failure ALTER COLUMN b SET STORAGE plain;
ALTER TABLE
=# INSERT INTO t_failure (SELECT FROM generate_series(1, 32)); -- use
up 32 line pointers on the first page.
INSERT 0 32
=# DELETE FROM t_failure;
DELETE 32
=# VACUUM (TRUNCATE OFF) t_failure; -- we now have a page that has
MaxHeapTupleSize > free space > 98% MaxHeapTupleSize
VACUUM
=# INSERT INTO t_failure (select 1, string_agg('1', '') from
generate_series(1, 8126));
PANIC:  failed to add tuple to page
server closed the connection unexpectedly
        This probably means the server terminated abnormally
        before or while processing the request.

A possible solution should always request at least the size of the
requested tuple, e.g.:
- targetFreeSpace = MaxHeapTupleSize - (MaxHeapTupleSize * 2 / 100);
+ targetFreeSpace = Max(len, MaxHeapTupleSize - (MaxHeapTupleSize * 2 / 100));


One different question I have, though, is why we can't "just" teach
vacuum to clean up trailing unused line pointers. As in, can't we trim
the line pointer array when vacuum detects that the trailing line
pointers on the page are all unused?

The only documentation that I could find that this doesn't happen is
in the comment on PageIndexTupleDelete and PageRepairFragmentation,
both not very descriptive on why we can't shrink the page->pd_linp
array. One is "Unlike heap pages, we compact out the line pointer for
the removed tuple." (Jan. 2002), and the other is "It doesn't remove
unused line pointers! Please don't change this." (Oct. 2000), but I
can't seem to find the documentation / conversations on the
implications that such shrinking would have.

With regards,

Matthias van de Meent.

  10. RE: non-HOT update not looking at FSM for large tuple update

    Floris Van Nee <florisvannee@optiver.com> — 2021-03-09T13:40:29Z

    Hi,

> 
> This patch fails to consider that len may be bigger than MaxHeapTupleSize *
> 0.98, which in this case triggers a reproducable
> PANIC:

Good catch! I've adapted the patch with your suggested fix.

> 
> One different question I have, though, is why we can't "just" teach vacuum
> to clean up trailing unused line pointers. As in, can't we trim the line pointer
> array when vacuum detects that the trailing line pointers on the page are all
> unused?
> 
> The only documentation that I could find that this doesn't happen is in the
> comment on PageIndexTupleDelete and PageRepairFragmentation, both not
> very descriptive on why we can't shrink the page->pd_linp array. One is
> "Unlike heap pages, we compact out the line pointer for the removed tuple."
> (Jan. 2002), and the other is "It doesn't remove unused line pointers! Please
> don't change this." (Oct. 2000), but I can't seem to find the documentation /
> conversations on the implications that such shrinking would have.
> 

This is an interesting alternative indeed. I also can't find any documentation/conversation about this and the message is rather cryptic.
Hopefully someone on the list still remembers the reasoning behind this rather cryptic comment in PageRepairFragmentation.

-Floris

  11. Re: non-HOT update not looking at FSM for large tuple update

    John Naylor <john.naylor@enterprisedb.com> — 2021-03-09T17:25:14Z

    On Tue, Mar 9, 2021 at 9:40 AM Floris Van Nee <florisvannee@optiver.com>
wrote:
>
> Hi,
>
> >
> > This patch fails to consider that len may be bigger than
MaxHeapTupleSize *
> > 0.98, which in this case triggers a reproducable
> > PANIC:
>
> Good catch! I've adapted the patch with your suggested fix.

Thank you both for testing and for the updated patch. It seems we should
add a regression test, but it's not clear which file it belongs in.
Possibly insert.sql?

> > One different question I have, though, is why we can't "just" teach
vacuum
> > to clean up trailing unused line pointers. As in, can't we trim the
line pointer
> > array when vacuum detects that the trailing line pointers on the page
are all
> > unused?

That seems like the proper fix, and I see you've started a thread for that.
I don't think that change in behavior would be backpatchable, but patch
here might have a chance at that.

--
John Naylor
EDB: http://www.enterprisedb.com

  12. Re: non-HOT update not looking at FSM for large tuple update

    John Naylor <john.naylor@enterprisedb.com> — 2021-03-09T17:38:57Z

    I wrote:

> That seems like the proper fix, and I see you've started a thread for
that. I don't think that change in behavior would be backpatchable, but
patch here might have a chance at that.

I remembered after the fact that truncating line pointers would only allow
for omitting the 2% slack logic (and has other benefits), but the rest of
this patch would be needed regardless.

--
John Naylor
EDB: http://www.enterprisedb.com

  13. Re: non-HOT update not looking at FSM for large tuple update

    Matthias van de Meent <boekewurm+postgres@gmail.com> — 2021-03-11T13:45:54Z

    On Tue, 9 Mar 2021 at 18:39, John Naylor <john.naylor@enterprisedb.com> wrote:
>
> I wrote:
>
> > That seems like the proper fix, and I see you've started a thread for that. I don't think that change in behavior would be backpatchable, but patch here might have a chance at that.
>
> I remembered after the fact that truncating line pointers would only allow for omitting the 2% slack logic (and has other benefits), but the rest of this patch would be needed regardless.

Regarding the 2% slack logic, could we change it to use increments of
line pointers instead? That makes it more clear what problem this
solution is trying to work around; that is to say line pointers not
(all) being truncated away. The currently subtracted value accounts
for the size of 40 line pointers on 8k-pages (~ 13.7% of
MaxHeapTuplesPerPage), and slightly higher fractions (up to 13.94%)
for larger page sizes. As the to-be-inserted tuple is already _at
least_ 10% of MaxHeapTupleSize when it hits this new code.

Also, even with this patch, we do FSM-requests of for sizes between
MaxHeapTupleSize - 2% and MaxHeapTupleSize, if len+saveFreeSpace falls
between those two numbers. I think we better clamp the fsm request
between `len` and `MaxHeapTupleSize - PAGE_SIZE_DEPENDENT_FACTOR`.

So, I sugges the following incremental patch:

 bool       needLock;
+ const Size maxPaddedFsmRequest = MaxHeapTupleSize -
(MaxHeapTuplesPerPage / 8 * sizeof(ItemIdData));
...
- if (len + saveFreeSpace > MaxHeapTupleSize)
+ if (len + saveFreeSpace > maxPaddedFsmRequest)
...
- targetFreeSpace = Max(len, MaxHeapTupleSize - (MaxHeapTupleSize * 2 / 100));
+ targetFreeSpace = Max(len, maxPaddedFsmRequest);
...

Other than this, I think this is a good fix.


With regards,

Matthias van de Meent.

  14. Re: non-HOT update not looking at FSM for large tuple update

    John Naylor <john.naylor@enterprisedb.com> — 2021-03-11T15:16:04Z

    On Thu, Mar 11, 2021 at 9:46 AM Matthias van de Meent <
boekewurm+postgres@gmail.com> wrote:

> Regarding the 2% slack logic, could we change it to use increments of
> line pointers instead? That makes it more clear what problem this
> solution is trying to work around; that is to say line pointers not
> (all) being truncated away. The currently subtracted value accounts

That makes sense.

> ...
> - if (len + saveFreeSpace > MaxHeapTupleSize)
> + if (len + saveFreeSpace > maxPaddedFsmRequest)
> ...
> - targetFreeSpace = Max(len, MaxHeapTupleSize - (MaxHeapTupleSize * 2 /
100));
> + targetFreeSpace = Max(len, maxPaddedFsmRequest);
> ...

If we have that convenient constant, it seems equivalent (I think) and a
bit more clear to write it this way, but I'm not wedded to it:

if (len + saveFreeSpace > MaxHeapTupleSize &&
    len <= maxPaddedFsmRequest)
{
    ...
    targetFreeSpace = maxPaddedFsmRequest;
}
else
    targetFreeSpace = len + saveFreeSpace;

Also, should I write a regression test for it? The test case is already
available, just no obvious place to put it.

--
John Naylor
EDB: http://www.enterprisedb.com

  15. Re: non-HOT update not looking at FSM for large tuple update

    Matthias van de Meent <boekewurm+postgres@gmail.com> — 2021-03-12T12:45:38Z

    On Thu, 11 Mar 2021 at 16:16, John Naylor <john.naylor@enterprisedb.com> wrote:
>
> On Thu, Mar 11, 2021 at 9:46 AM Matthias van de Meent <boekewurm+postgres@gmail.com> wrote:
>
> > Regarding the 2% slack logic, could we change it to use increments of
> > line pointers instead? That makes it more clear what problem this
> > solution is trying to work around; that is to say line pointers not
> > (all) being truncated away. The currently subtracted value accounts
>
> That makes sense.
>
> > ...
> > - if (len + saveFreeSpace > MaxHeapTupleSize)
> > + if (len + saveFreeSpace > maxPaddedFsmRequest)
> > ...
> > - targetFreeSpace = Max(len, MaxHeapTupleSize - (MaxHeapTupleSize * 2 / 100));
> > + targetFreeSpace = Max(len, maxPaddedFsmRequest);
> > ...
>
> If we have that convenient constant, it seems equivalent (I think) and a bit more clear to write it this way, but I'm not wedded to it:
>
> if (len + saveFreeSpace > MaxHeapTupleSize &&
>     len <= maxPaddedFsmRequest)
> {
>     ...
>     targetFreeSpace = maxPaddedFsmRequest;
> }

+ else if (len > maxPaddedFsmRequest
+ {
+   /* request len amount of space; it might still fit on
not-quite-empty pages */
+   targetFreeSpace = len;
+ }

If this case isn't added, the lower else branch will fail to find
fitting pages for len > maxPaddedFsmRequest tuples; potentially
extending the relation when there is actually still enough space
available.

> else
>     targetFreeSpace = len + saveFreeSpace;

> Also, should I write a regression test for it? The test case is already available, just no obvious place to put it.

I think it would be difficult to write tests that exhibit the correct
behaviour on BLCKSZ != 8196. On the other hand, I see there are some
tests that explicitly call out that they expect BLCKSZ to be 8192, so
that has not really been a hard block before.

The previous code I sent had initial INSERT + DELETE + VACUUM. These
statements can be replaced with `INSERT INTO t_failure (b) VALUES
(repeat('1', 95)); VACUUM;` for simplicity. The vacuum is still needed
to populate the FSM for the new page.

With regards,

Matthias van de Meent

  16. Re: non-HOT update not looking at FSM for large tuple update

    John Naylor <john.naylor@enterprisedb.com> — 2021-03-17T20:51:48Z

    On Fri, Mar 12, 2021 at 8:45 AM Matthias van de Meent <
boekewurm+postgres@gmail.com> wrote:
>
> If this case isn't added, the lower else branch will fail to find
> fitting pages for len > maxPaddedFsmRequest tuples; potentially
> extending the relation when there is actually still enough space
> available.

Okay, with that it looks better to go back to using Max().

Also in v4:

- With the separate constant you suggested, I split up the comment into two
parts.
- I've added a regression test to insert.sql similar to your earlier test,
but we cannot use vacuum, since in parallel tests there could still be
tuples visible to other transactions. It's still possible to test
almost-all-free by inserting a small tuple.
- Draft commit message

--
John Naylor
EDB: http://www.enterprisedb.com

  17. Re: non-HOT update not looking at FSM for large tuple update

    Matthias van de Meent <boekewurm+postgres@gmail.com> — 2021-03-18T21:30:36Z

    On Wed, 17 Mar 2021 at 21:52, John Naylor <john.naylor@enterprisedb.com> wrote:
>
> On Fri, Mar 12, 2021 at 8:45 AM Matthias van de Meent <boekewurm+postgres@gmail.com> wrote:
> >
> > If this case isn't added, the lower else branch will fail to find
> > fitting pages for len > maxPaddedFsmRequest tuples; potentially
> > extending the relation when there is actually still enough space
> > available.
>
> Okay, with that it looks better to go back to using Max().
>
> Also in v4:
>
> - With the separate constant you suggested, I split up the comment into two parts.

> +     * The minimum space on a page for it to be considered "empty" regardless
> +     * of fillfactor. We base this on MaxHeapTupleSize, minus a small amount
> +     * of slack. We allow slack equal to 1/8 the maximum space that could be
> +     * taken by line pointers, which is somewhat arbitrary.

> +     * We want to allow inserting a large tuple into an empty page even if
> +     * that would violate the fillfactor. Otherwise, we would unnecessarily
> +     * extend the relation. Instead, ask the FSM for maxPaddedFsmRequest
> +     * bytes. This will allow it to return a page that is not quite empty
> +     * because of unused line pointers

How about

+    * Because pages that have no items left can still have space allocated
+    * to item pointers, we consider pages "empty" for FSM requests when they
+    * have at most 1/8 of their MaxHeapTuplesPerPage item pointers' space
+    * allocated. This is a somewhat arbitrary number, but should prevent
+    * most unnecessary relation extensions due to not finding "empty" pages
+    * while inserting combinations of large tuples with low fillfactors.

+    * When the free space to be requested from the FSM is greater than
+    * maxPaddedFsmRequest, this is considered equivalent to requesting
+    * insertion on an "empty" page, so instead of failing to find a page
+    * with more empty space than an "empty" page and extend the relation,
+    * we try to find a page which is considered "emtpy".

This is slightly more verbose, but I think this clarifies the
reasoning why we need this a bit better. Feel free to reject or adapt
as needed.

> - I've added a regression test to insert.sql similar to your earlier test, but we cannot use vacuum, since in parallel tests there could still be tuples visible to other transactions. It's still possible to test almost-all-free by inserting a small tuple.
> - Draft commit message

Apart from these mainly readability changes in comments, I think this is ready.

> --
> John Naylor
> EDB: http://www.enterprisedb.com

  18. Re: non-HOT update not looking at FSM for large tuple update

    John Naylor <john.naylor@enterprisedb.com> — 2021-03-19T18:16:22Z

    On Thu, Mar 18, 2021 at 5:30 PM Matthias van de Meent <
boekewurm+postgres@gmail.com> wrote:
>
> > +     * The minimum space on a page for it to be considered "empty"
regardless
> > +     * of fillfactor. We base this on MaxHeapTupleSize, minus a small
amount
> > +     * of slack. We allow slack equal to 1/8 the maximum space that
could be
> > +     * taken by line pointers, which is somewhat arbitrary.
>
> > +     * We want to allow inserting a large tuple into an empty page
even if
> > +     * that would violate the fillfactor. Otherwise, we would
unnecessarily
> > +     * extend the relation. Instead, ask the FSM for
maxPaddedFsmRequest
> > +     * bytes. This will allow it to return a page that is not quite
empty
> > +     * because of unused line pointers
>
> How about
>
> +    * Because pages that have no items left can still have space
allocated
> +    * to item pointers, we consider pages "empty" for FSM requests when
they
> +    * have at most 1/8 of their MaxHeapTuplesPerPage item pointers' space
> +    * allocated. This is a somewhat arbitrary number, but should prevent
> +    * most unnecessary relation extensions due to not finding "empty"
pages
> +    * while inserting combinations of large tuples with low fillfactors.
>
> +    * When the free space to be requested from the FSM is greater than
> +    * maxPaddedFsmRequest, this is considered equivalent to requesting
> +    * insertion on an "empty" page, so instead of failing to find a page
> +    * with more empty space than an "empty" page and extend the relation,
> +    * we try to find a page which is considered "emtpy".
>
> This is slightly more verbose, but I think this clarifies the
> reasoning why we need this a bit better. Feel free to reject or adapt
> as needed.

I like this in general, but still has some rough edges. I've made another
attempt in v5 incorporating your suggestions. Let me know what you think.

--
John Naylor
EDB: http://www.enterprisedb.com

  19. Re: non-HOT update not looking at FSM for large tuple update

    Matthias van de Meent <boekewurm+postgres@gmail.com> — 2021-03-19T18:42:08Z

    On Fri, 19 Mar 2021 at 19:16, John Naylor <john.naylor@enterprisedb.com> wrote:
>
> On Thu, Mar 18, 2021 at 5:30 PM Matthias van de Meent <boekewurm+postgres@gmail.com> wrote:
> >
> > This is slightly more verbose, but I think this clarifies the
> > reasoning why we need this a bit better. Feel free to reject or adapt
> > as needed.
>
> I like this in general, but still has some rough edges. I've made another attempt in v5 incorporating your suggestions. Let me know what you think.

That is indeed better.

I believe this is ready, so I've marked it as RFC in the commitfest application.

With regards,

Matthias van de Meent.

  20. Re: non-HOT update not looking at FSM for large tuple update

    Noah Misch <noah@leadboat.com> — 2021-03-27T07:00:31Z

    I gather this is important when large_upd_rate=rate(cross-page update bytes
for tuples larger than fillfactor) exceeds small_ins_rate=rate(insert bytes
for tuples NOT larger than fillfactor).  That is a plausible outcome when
inserts are rare, and table bloat then accrues at
large_upd_rate-small_ins_rate.  I agree this patch improves behavior.

Does anyone have a strong opinion on whether to back-patch?  I am weakly
inclined not to back-patch, because today's behavior might happen to perform
better when large_upd_rate-small_ins_rate<0.  Besides the usual choices of
back-patching or not back-patching, we could back-patch with a stricter
threshold.  Suppose we accepted pages for larger-than-fillfactor tuples when
the pages have at least
BLCKSZ-SizeOfPageHeaderData-sizeof(ItemIdData)-MAXALIGN(MAXALIGN(SizeofHeapTupleHeader)+1)+1
bytes of free space.  That wouldn't reuse a page containing a one-column
tuple, but it would reuse a page having up to eight line pointers.

On Fri, Mar 19, 2021 at 02:16:22PM -0400, John Naylor wrote:
> --- a/src/backend/access/heap/hio.c
> +++ b/src/backend/access/heap/hio.c
> @@ -335,11 +335,24 @@ RelationGetBufferForTuple(Relation relation, Size len,

> +	const Size	maxPaddedFsmRequest = MaxHeapTupleSize -
> +	(MaxHeapTuplesPerPage / 8 * sizeof(ItemIdData));

In evaluating whether this is a good choice of value, I think about the
expected page lifecycle.  A tuple barely larger than fillfactor (roughly
len=1+BLCKSZ*fillfactor/100) will start on a roughly-empty page.  As long as
the tuple exists, the server will skip that page for inserts.  Updates can
cause up to floor(99/fillfactor) same-size versions of the tuple to occupy the
page simultaneously, creating that many line pointers.  At the fillfactor=10
minimum, it's good to accept otherwise-empty pages having at least nine line
pointers, so a page can restart the aforementioned lifecycle.  Tolerating even
more line pointers helps when updates reduce tuple size or when the page was
used for smaller tuples before it last emptied.  At the BLCKSZ=8192 default,
this maxPaddedFsmRequest allows 36 line pointers (good or somewhat high).  At
the BLCKSZ=1024 minimum, it allows 4 line pointers (low).  At the BLCKSZ=32768
maximum, 146 (likely excessive).  I'm not concerned about optimizing
non-default block sizes, so let's keep your proposal.

Comments and the maxPaddedFsmRequest variable name use term "fsm" for things
not specific to the FSM.  For example, the patch's test case doesn't use the
FSM.  (That is fine.  Ordinarily, RelationGetTargetBlock() furnishes its
block.  Under CLOBBER_CACHE_ALWAYS, the "try the last page" logic does so.  An
FSM-using test would contain a VACUUM.)  I plan to commit the attached
version; compared to v5, it updates comments and renames this variable.

Thanks,
nm

  21. RE: non-HOT update not looking at FSM for large tuple update

    Floris Van Nee <florisvannee@optiver.com> — 2021-03-27T10:24:00Z

    Hi Noah,

Thanks for taking a look at this patch.

> 
> In evaluating whether this is a good choice of value, I think about the
> expected page lifecycle.  A tuple barely larger than fillfactor (roughly
> len=1+BLCKSZ*fillfactor/100) will start on a roughly-empty page.  As long as
> the tuple exists, the server will skip that page for inserts.  Updates can cause
> up to floor(99/fillfactor) same-size versions of the tuple to occupy the page
> simultaneously, creating that many line pointers.  At the fillfactor=10
> minimum, it's good to accept otherwise-empty pages having at least nine line
> pointers, so a page can restart the aforementioned lifecycle.  Tolerating even
> more line pointers helps when updates reduce tuple size or when the page
> was used for smaller tuples before it last emptied.  At the BLCKSZ=8192
> default, this maxPaddedFsmRequest allows 36 line pointers (good or
> somewhat high).  At the BLCKSZ=1024 minimum, it allows 4 line pointers
> (low).  At the BLCKSZ=32768 maximum, 146 (likely excessive).  I'm not
> concerned about optimizing non-default block sizes, so let's keep your
> proposal.
> 

Agreed. You briefly mention this already, but the case that caused me to report this was exactly the one where under normal circumstances each UPDATE would be small. However, in rare cases, the tuple that is updated grows in size to 1k bytes (the specific case we encountered sometimes would under specific circumstances write extra info in a field, which would otherwise be NULL). Suppose that this 1k UPDATE does not fit into the current page (so no HOT update), then a new page would be created (HEAD behavior). However, it is very likely that the next updates to this same tuple will be the regular size again. This causes the higher number of line pointers on the page.

-Floris

  22. Re: non-HOT update not looking at FSM for large tuple update

    John Naylor <john.naylor@enterprisedb.com> — 2021-03-27T15:26:47Z

    On Sat, Mar 27, 2021 at 3:00 AM Noah Misch <noah@leadboat.com> wrote:
>
> Does anyone have a strong opinion on whether to back-patch?  I am weakly
> inclined not to back-patch, because today's behavior might happen to
perform
> better when large_upd_rate-small_ins_rate<0.

It's not a clear case. The present behavior is clearly a bug, but only
manifests in rare situations. The risk of the fix affecting other
situations is not zero, as you mention, but (thinking briefly about this
and I could be wrong) the consequences don't seem as big as the reported
case of growing table size.

> Besides the usual choices of
> back-patching or not back-patching, we could back-patch with a stricter
> threshold.  Suppose we accepted pages for larger-than-fillfactor tuples
when
> the pages have at least
>
BLCKSZ-SizeOfPageHeaderData-sizeof(ItemIdData)-MAXALIGN(MAXALIGN(SizeofHeapTupleHeader)+1)+1
> bytes of free space.  That wouldn't reuse a page containing a one-column
> tuple, but it would reuse a page having up to eight line pointers.

I'm not sure how much that would help in the reported case that started
this thread.

> Comments and the maxPaddedFsmRequest variable name use term "fsm" for
things
> not specific to the FSM.  For example, the patch's test case doesn't use
the
> FSM.  (That is fine.  Ordinarily, RelationGetTargetBlock() furnishes its
> block.  Under CLOBBER_CACHE_ALWAYS, the "try the last page" logic does
so.  An
> FSM-using test would contain a VACUUM.)  I plan to commit the attached
> version; compared to v5, it updates comments and renames this variable.

Looks good to me, thanks!

--
John Naylor
EDB: http://www.enterprisedb.com

  23. Re: non-HOT update not looking at FSM for large tuple update

    Noah Misch <noah@leadboat.com> — 2021-03-27T20:32:57Z

    On Sat, Mar 27, 2021 at 11:26:47AM -0400, John Naylor wrote:
> On Sat, Mar 27, 2021 at 3:00 AM Noah Misch <noah@leadboat.com> wrote:
> > Does anyone have a strong opinion on whether to back-patch?  I am weakly
> > inclined not to back-patch, because today's behavior might happen to perform
> > better when large_upd_rate-small_ins_rate<0.
> 
> It's not a clear case. The present behavior is clearly a bug, but only
> manifests in rare situations. The risk of the fix affecting other situations
> is not zero, as you mention, but (thinking briefly about this and I could be
> wrong) the consequences don't seem as big as the reported case of growing
> table size.

I agree sites that are hurting now will see a net benefit.  I can call it a
bug that we treat just-extended pages differently from existing
zero-line-pointer pages (e.g. pages left by RelationAddExtraBlocks()).
Changing how we treat pages having 100 bytes of data feels different to me.
It's more like a policy decision, not a clear bug fix.

I'm open to back-patching, but I plan to do so only if a few people report
being firmly in favor.

> > Besides the usual choices of
> > back-patching or not back-patching, we could back-patch with a stricter
> > threshold.  Suppose we accepted pages for larger-than-fillfactor tuples when
> > the pages have at least
> > BLCKSZ-SizeOfPageHeaderData-sizeof(ItemIdData)-MAXALIGN(MAXALIGN(SizeofHeapTupleHeader)+1)+1
> > bytes of free space.  That wouldn't reuse a page containing a one-column
> > tuple, but it would reuse a page having up to eight line pointers.
> 
> I'm not sure how much that would help in the reported case that started this thread.

I'm not sure, either.  The thread email just before yours (27 Mar 2021
10:24:00 +0000) does suggest it would help less than the main proposal.