Horton

Thread

Commits

  1. Improve stability of test with vacuum_truncate in reloptions.sql

  1. Flaky vacuum truncate test in reloptions.sql

    Arseny Sher <a.sher@postgrespro.ru> — 2021-03-29T22:58:50Z

    Hi,

I rarely observe failure of vacuum with truncation test in
reloptions.sql, i.e. the truncation doesn't happen:

--- ../../src/test/regress/expected/reloptions.out      2020-04-16 12:37:17.749547401 +0300
+++ ../../src/test/regress/results/reloptions.out       2020-04-17 00:14:58.999211750 +0300
@@ -131,7 +131,7 @@
 SELECT pg_relation_size('reloptions_test') = 0;
  ?column?
 ----------
- t
+ f
 (1 row)

Intimate reading of lazy_scan_heap says that the failure indeed might
happen; if ConditionalLockBufferForCleanup couldn't lock the buffer and
either the buffer doesn't need freezing or vacuum is not aggressive, we
don't insist on close inspection of the page contents and count it as
nonempty according to lazy_check_needs_freeze. It means the page is
regarded as such even if it contains only garbage (but occupied) ItemIds,
which is the case of the test. And of course this allegedly nonempty
page prevents the truncation. Obvious competitors for the page are
bgwriter/checkpointer; the chances of a simultaneous attack are small
but they exist.

A simple fix is to perform aggressive VACUUM FREEZE, as attached.

I'm a bit puzzled that I've ever seen this only when running regression
tests under our multimaster. While multimaster contains a fair amount of
C code, I don't see how any of it can interfere with the vacuuming
business here. I can't say I did my best to create the repoduction
though -- the explanation above seems to be enough.


--
Arseny Sher
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

  2. Re: Flaky vacuum truncate test in reloptions.sql

    Michael Paquier <michael@paquier.xyz> — 2021-03-30T07:12:19Z

    On Tue, Mar 30, 2021 at 01:58:50AM +0300, Arseny Sher wrote:
> Intimate reading of lazy_scan_heap says that the failure indeed might
> happen; if ConditionalLockBufferForCleanup couldn't lock the buffer and
> either the buffer doesn't need freezing or vacuum is not aggressive, we
> don't insist on close inspection of the page contents and count it as
> nonempty according to lazy_check_needs_freeze. It means the page is
> regarded as such even if it contains only garbage (but occupied) ItemIds,
> which is the case of the test. And of course this allegedly nonempty
> page prevents the truncation. Obvious competitors for the page are
> bgwriter/checkpointer; the chances of a simultaneous attack are small
> but they exist.

Yep, this is the same problem as the one discussed for c2dc1a7, where
a concurrent checkpoint may cause a page to be skipped, breaking the
test.

> I'm a bit puzzled that I've ever seen this only when running regression
> tests under our multimaster. While multimaster contains a fair amount of
> C code, I don't see how any of it can interfere with the vacuuming
> business here. I can't say I did my best to create the repoduction
> though -- the explanation above seems to be enough.

Why not just using DISABLE_PAGE_SKIPPING instead here?
--
Michael

  3. Re: Flaky vacuum truncate test in reloptions.sql

    Arseny Sher <a.sher@postgrespro.ru> — 2021-03-30T13:22:18Z

    On 3/30/21 10:12 AM, Michael Paquier wrote:

 > Yep, this is the same problem as the one discussed for c2dc1a7, where
 > a concurrent checkpoint may cause a page to be skipped, breaking the
 > test.

Indeed, Alexander Lakhin pointed me to that commit after I wrote the 
message.

 > Why not just using DISABLE_PAGE_SKIPPING instead here?

I think this is not enough. DISABLE_PAGE_SKIPPING disables vm consulting 
(sets
aggressive=true in the routine); however, if the page is locked and
lazy_check_needs_freeze says there is nothing to freeze on it, we again 
don't
look at its contents closely.


-- cheers, arseny

  4. Re: Flaky vacuum truncate test in reloptions.sql

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-03-31T13:17:09Z

    On Tue, Mar 30, 2021 at 10:22 PM Arseny Sher <a.sher@postgrespro.ru> wrote:
>
> On 3/30/21 10:12 AM, Michael Paquier wrote:
>
>  > Yep, this is the same problem as the one discussed for c2dc1a7, where
>  > a concurrent checkpoint may cause a page to be skipped, breaking the
>  > test.
>
> Indeed, Alexander Lakhin pointed me to that commit after I wrote the
> message.
>
>  > Why not just using DISABLE_PAGE_SKIPPING instead here?
>
> I think this is not enough. DISABLE_PAGE_SKIPPING disables vm consulting
> (sets
> aggressive=true in the routine); however, if the page is locked and
> lazy_check_needs_freeze says there is nothing to freeze on it, we again
> don't
> look at its contents closely.

Right.

Is it better to add FREEZE to the first "VACUUM reloptions_test;" as well?

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

  5. Re: Flaky vacuum truncate test in reloptions.sql

    Arseny Sher <a.sher@postgrespro.ru> — 2021-03-31T13:39:39Z

    On 3/31/21 4:17 PM, Masahiko Sawada wrote:

 > Is it better to add FREEZE to the first "VACUUM reloptions_test;" as 
well?

I don't think this matters much, as it tests the contrary and the 
probability of
successful test passing (in case of theoretical bug making vacuum to 
truncate
non-empty relation) becomes stunningly small. But adding it wouldn't hurt
either.

-- cheers, arseny

  6. Re: Flaky vacuum truncate test in reloptions.sql

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-04-01T02:33:54Z

    On Wed, Mar 31, 2021 at 10:39 PM Arseny Sher <a.sher@postgrespro.ru> wrote:
>
>
> On 3/31/21 4:17 PM, Masahiko Sawada wrote:
>
>  > Is it better to add FREEZE to the first "VACUUM reloptions_test;" as
> well?
>
> I don't think this matters much, as it tests the contrary and the
> probability of
> successful test passing (in case of theoretical bug making vacuum to
> truncate
> non-empty relation) becomes stunningly small. But adding it wouldn't hurt
> either.

I was concerned a bit that without FREEZE in the first VACUUM we could
not test it properly because the table could not be truncated because
either vacuum_truncate is off or the page is skipped.

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

  7. Re: Flaky vacuum truncate test in reloptions.sql

    Arseny Sher <a.sher@postgrespro.ru> — 2021-04-01T03:08:28Z

    Masahiko Sawada <sawada.mshk@gmail.com> writes:

>> I don't think this matters much, as it tests the contrary and the
>> probability of
>> successful test passing (in case of theoretical bug making vacuum to
>> truncate
>> non-empty relation) becomes stunningly small. But adding it wouldn't hurt
>> either.
>
> I was concerned a bit that without FREEZE in the first VACUUM we could
> not test it properly because the table could not be truncated because
> either vacuum_truncate is off

FREEZE won't help us there.

> or the page is skipped.

You mean at the same time there is a potential bug in vacuum which would
force the truncation of non-empy relation if the page wasn't locked?
That would mean the chance of test getting passed even single time is
close to 0, as currently the chance of its failure is close to 1.


-- cheers, arseny

  8. Re: Flaky vacuum truncate test in reloptions.sql

    Arseny Sher <a.sher@postgrespro.ru> — 2021-04-01T03:11:50Z

    Arseny Sher <a.sher@postgrespro.ru> writes:

> as currently the chance of its failure is close to 1.

A typo, to 0 too, of course.

  9. Re: Flaky vacuum truncate test in reloptions.sql

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-04-01T03:52:21Z

    On Thu, Apr 1, 2021 at 12:08 PM Arseny Sher <a.sher@postgrespro.ru> wrote:
>
>
> Masahiko Sawada <sawada.mshk@gmail.com> writes:
>
> >> I don't think this matters much, as it tests the contrary and the
> >> probability of
> >> successful test passing (in case of theoretical bug making vacuum to
> >> truncate
> >> non-empty relation) becomes stunningly small. But adding it wouldn't hurt
> >> either.
> >
> > I was concerned a bit that without FREEZE in the first VACUUM we could
> > not test it properly because the table could not be truncated because
> > either vacuum_truncate is off
>
> FREEZE won't help us there.
>
> > or the page is skipped.
>
> You mean at the same time there is a potential bug in vacuum which would
> force the truncation of non-empy relation if the page wasn't locked?

Just to be clear the context, I’m mentioning the following test case:

CREATE TABLE reloptions_test(i INT NOT NULL, j text)
        WITH (vacuum_truncate=false,
        toast.vacuum_truncate=false,
        autovacuum_enabled=false);
SELECT reloptions FROM pg_class WHERE oid = 'reloptions_test'::regclass;
INSERT INTO reloptions_test VALUES (1, NULL), (NULL, NULL);
VACUUM reloptions_test;
SELECT pg_relation_size('reloptions_test') > 0;

What I meant is that without FREEZE option, there are two possible
cases where the table is not truncated (i.g.,
pg_relation_size('reloptions_test') > 0 is true): the page got empty
by vacuum but is not truncated because of vacuum_truncate = false, and
the page could not be vacuumed (i.g., tuples remain in the page)
because the page is skipped due to conflict on cleanup lock on the
page. This test is intended to test the former case. I guess adding
FREEZE will prevent the latter case.

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

  10. Re: Flaky vacuum truncate test in reloptions.sql

    Michael Paquier <michael@paquier.xyz> — 2021-04-01T04:28:19Z

    On Thu, Apr 01, 2021 at 12:52:21PM +0900, Masahiko Sawada wrote:
> Just to be clear the context, I’m mentioning the following test case:

(Coming back a couple of emails later, where indeed I forgot about the
business with lazy_check_needs_freeze() that could cause a page to be
skipped even if DISABLE_PAGE_SKIPPING is used.)

> What I meant is that without FREEZE option, there are two possible
> cases where the table is not truncated (i.g.,
> pg_relation_size('reloptions_test') > 0 is true): the page got empty
> by vacuum but is not truncated because of vacuum_truncate = false, and
> the page could not be vacuumed (i.g., tuples remain in the page)
> because the page is skipped due to conflict on cleanup lock on the
> page. This test is intended to test the former case. I guess adding
> FREEZE will prevent the latter case.

What you are writing here makes sense to me.  Looking at the test, it
is designed to test vacuum_truncate, aka that the behavior we want to
stress (your former case here) gets stressed all the time, so adding
the options to avoid the latter case all the time is an improvement.
And this, even if the latter case does not actually cause a diff and
it has a small chance to happen in practice.

It would be good to add a comment explaining why the options are
added (aka just don't skip any pages).
--
Michael

  11. Re: Flaky vacuum truncate test in reloptions.sql

    Arseny Sher <a.sher@postgrespro.ru> — 2021-04-01T07:58:25Z

    Michael Paquier <michael@paquier.xyz> writes:

> On Thu, Apr 01, 2021 at 12:52:21PM +0900, Masahiko Sawada wrote:
>> Just to be clear the context, I’m mentioning the following test case:

Sorry, I misremembered the test and assumed the table is non-empty there
while it is empty but vacuum_truncate is disabled. Still, this doesn't
change my conclusion of freezing being not a big deal there due to small
chance of locked page. Anyway, let's finish with this.

> What you are writing here makes sense to me.  Looking at the test, it
> is designed to test vacuum_truncate, aka that the behavior we want to
> stress (your former case here) gets stressed all the time, so adding
> the options to avoid the latter case all the time is an improvement.
> And this, even if the latter case does not actually cause a diff and
> it has a small chance to happen in practice.
>
> It would be good to add a comment explaining why the options are
> added (aka just don't skip any pages).

How about the attached?


-- cheers, arseny

  12. Re: Flaky vacuum truncate test in reloptions.sql

    Michael Paquier <michael@paquier.xyz> — 2021-04-01T08:49:03Z

    On Thu, Apr 01, 2021 at 10:58:25AM +0300, Arseny Sher wrote:
> How about the attached?

Sounds fine to me.  Sawada-san?
--
Michael

  13. Re: Flaky vacuum truncate test in reloptions.sql

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-04-01T13:54:29Z

    On Thu, Apr 1, 2021 at 5:49 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Thu, Apr 01, 2021 at 10:58:25AM +0300, Arseny Sher wrote:
> > How about the attached?

Thank you for updating the patch!

> Sounds fine to me.  Sawada-san?

Looks good to me too.

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

  14. Re: Flaky vacuum truncate test in reloptions.sql

    Michael Paquier <michael@paquier.xyz> — 2021-04-02T00:46:22Z

    On Thu, Apr 01, 2021 at 10:54:29PM +0900, Masahiko Sawada wrote:
> Looks good to me too.

Okay, applied and back-patched down to 12 then.
--
Michael

  15. Re: Flaky vacuum truncate test in reloptions.sql

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-04-02T02:57:00Z

    On Fri, Apr 2, 2021 at 9:46 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Thu, Apr 01, 2021 at 10:54:29PM +0900, Masahiko Sawada wrote:
> > Looks good to me too.
>
> Okay, applied and back-patched down to 12 then.

Thank you!


Regards,

--
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

  16. Re: Flaky vacuum truncate test in reloptions.sql

    Arseny Sher <a.sher@postgrespro.ru> — 2021-04-04T20:00:25Z

    On Fri, Apr 2, 2021 at 9:46 AM Michael Paquier <michael@paquier.xyz> wrote:

> Okay, applied and back-patched down to 12 then.

Thank you both. Unfortunately and surprisingly, the test still fails
(perhaps even rarer, once in several hundred runs) under
multimaster. After scratching the head for some more time, it seems to
me the following happens: not only vacuum encounters locked page, but
also there exist a concurrent backend (as the parallel schedule is run)
who holds back oldestxmin keeping it less than xid of transaction which
did the insertion

INSERT INTO reloptions_test VALUES (1, NULL), (NULL, NULL);

FreezeLimit can't be higher than oldestxmin, so lazy_check_needs_freeze
decides there is nothing to freeze on the page. multimaster commits are
quite heavy, which apparently shifts the timings making the issue more
likely.

Currently we are testing the rather funny attached patch which forces
all such old-snapshot-holders to finish. It is crutchy, but I doubt we
want to change vacuum logic (e.g. checking tuple liveness in
lazy_check_needs_freeze) due to this issue. (it is especially crutchy in
xid::bigint casts, but wraparound is hardly expected in regression tests
run).


-- cheers, arseny

  17. Re: Flaky vacuum truncate test in reloptions.sql

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-04-05T11:55:04Z

    On Mon, Apr 5, 2021 at 5:00 AM Arseny Sher <a.sher@postgrespro.ru> wrote:
>
>
> On Fri, Apr 2, 2021 at 9:46 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> > Okay, applied and back-patched down to 12 then.
>
> Thank you both. Unfortunately and surprisingly, the test still fails
> (perhaps even rarer, once in several hundred runs) under
> multimaster. After scratching the head for some more time, it seems to
> me the following happens: not only vacuum encounters locked page, but
> also there exist a concurrent backend (as the parallel schedule is run)
> who holds back oldestxmin keeping it less than xid of transaction which
> did the insertion
>
> INSERT INTO reloptions_test VALUES (1, NULL), (NULL, NULL);
>
> FreezeLimit can't be higher than oldestxmin, so lazy_check_needs_freeze
> decides there is nothing to freeze on the page. multimaster commits are
> quite heavy, which apparently shifts the timings making the issue more
> likely.
>
> Currently we are testing the rather funny attached patch which forces
> all such old-snapshot-holders to finish. It is crutchy, but I doubt we
> want to change vacuum logic (e.g. checking tuple liveness in
> lazy_check_needs_freeze) due to this issue. (it is especially crutchy in
> xid::bigint casts, but wraparound is hardly expected in regression tests
> run).

Or maybe we can remove reloptions.sql test from the parallel group.
BTW I wonder if the following tests in vacuum.sql test also have the
same issue (page skipping and oldestxmin):

-- TRUNCATE option
CREATE TABLE vac_truncate_test(i INT NOT NULL, j text)
        WITH (vacuum_truncate=true, autovacuum_enabled=false);
INSERT INTO vac_truncate_test VALUES (1, NULL), (NULL, NULL);
VACUUM (TRUNCATE FALSE) vac_truncate_test;
SELECT pg_relation_size('vac_truncate_test') > 0;
VACUUM vac_truncate_test;
SELECT pg_relation_size('vac_truncate_test') = 0;
VACUUM (TRUNCATE FALSE, FULL TRUE) vac_truncate_test;
DROP TABLE vac_truncate_test;

Should we add FREEZE to those tests as well?

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/