Content-Type: multipart/mixed; boundary="------------ZWFMzUiDj8jnB5QVuU0CAxHw"
Message-ID: <3df2b582-dc1c-46b6-99b6-38eddd1b2784@iki.fi>
Date: Fri, 8 Mar 2024 15:49:47 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: Confine vacuum skip logic to lazy_scan_skip
Content-Language: en-US
To: vignesh C <vignesh21@gmail.com>, Andres Freund <andres@anarazel.de>,
 Pg Hackers <pgsql-hackers@postgresql.org>,
 Thomas Munro <thomas.munro@gmail.com>, Nazir Bilal Yavuz
 <byavuz81@gmail.com>, Robert Haas <robertmhaas@gmail.com>,
 Peter Geoghegan <pg@bowt.ie>
References: <CAAKRu_Yf3gvXGcCnqqfoq0Q8LX8UM-e-qbm_B1LeZh60f8WhWA@mail.gmail.com>
 <CAAKRu_aK68L=YbMf4rtksxk-kz4dnmMZmjJwohjVf0FTSpgXdw@mail.gmail.com>
 <20240104202309.77h5llrambkl5a3m@awork3.anarazel.de>
 <CAAKRu_b9kbXeK05_jTqBayhsRNeruyWHD6i8RPWoVtGxSV4D9g@mail.gmail.com>
 <CALDaNm3y-kxuJzgqHrJUVO-B=JPE3tXJazoowEkKL745aZH6Zw@mail.gmail.com>
 <CAAKRu_ZuYsf4MNGzm7_KvG0SKsu-EtYRhFHy2x3q0heX8KLMTg@mail.gmail.com>
 <CAAKRu_bQgf4e9OveCQX++zTEJLKbZbqLeapA7SNkNuMjxcCO1w@mail.gmail.com>
 <1eeccf12-d5d1-4b7e-b88b-7342410129d7@iki.fi>
 <20240307030023.2gajgexeelqtohnu@liskov>
 <20240308004614.xn7x3k6qz7jw7ljx@liskov>
From: Heikki Linnakangas <hlinnaka@iki.fi>
In-Reply-To: <20240308004614.xn7x3k6qz7jw7ljx@liskov>
Archived-At: <https://www.postgresql.org/message-id/3df2b582-dc1c-46b6-99b6-38eddd1b2784%40iki.fi>
Precedence: bulk

This is a multi-part message in MIME format.
--------------ZWFMzUiDj8jnB5QVuU0CAxHw
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

On 08/03/2024 02:46, Melanie Plageman wrote:
> On Wed, Mar 06, 2024 at 10:00:23PM -0500, Melanie Plageman wrote:
>>> I feel heap_vac_scan_get_next_block() function could use some love. Maybe
>>> just some rewording of the comments, or maybe some other refactoring; not
>>> sure. But I'm pretty happy with the function signature and how it's called.
> 
> I've cleaned up the comments on heap_vac_scan_next_block() in the first
> couple patches (not so much in the streaming read user). Let me know if
> it addresses your feelings or if I should look for other things I could
> change.

Thanks, that is better. I think I now finally understand how the 
function works, and now I can see some more issues and refactoring 
opportunities :-).

Looking at current lazy_scan_skip() code in 'master', one thing now 
caught my eye (and it's the same with your patches):

> 	*next_unskippable_allvis = true;
> 	while (next_unskippable_block < rel_pages)
> 	{
> 		uint8		mapbits = visibilitymap_get_status(vacrel->rel,
> 													   next_unskippable_block,
> 													   vmbuffer);
> 
> 		if ((mapbits & VISIBILITYMAP_ALL_VISIBLE) == 0)
> 		{
> 			Assert((mapbits & VISIBILITYMAP_ALL_FROZEN) == 0);
> 			*next_unskippable_allvis = false;
> 			break;
> 		}
> 
> 		/*
> 		 * Caller must scan the last page to determine whether it has tuples
> 		 * (caller must have the opportunity to set vacrel->nonempty_pages).
> 		 * This rule avoids having lazy_truncate_heap() take access-exclusive
> 		 * lock on rel to attempt a truncation that fails anyway, just because
> 		 * there are tuples on the last page (it is likely that there will be
> 		 * tuples on other nearby pages as well, but those can be skipped).
> 		 *
> 		 * Implement this by always treating the last block as unsafe to skip.
> 		 */
> 		if (next_unskippable_block == rel_pages - 1)
> 			break;
> 
> 		/* DISABLE_PAGE_SKIPPING makes all skipping unsafe */
> 		if (!vacrel->skipwithvm)
> 		{
> 			/* Caller shouldn't rely on all_visible_according_to_vm */
> 			*next_unskippable_allvis = false;
> 			break;
> 		}
> 
> 		/*
> 		 * Aggressive VACUUM caller can't skip pages just because they are
> 		 * all-visible.  They may still skip all-frozen pages, which can't
> 		 * contain XIDs < OldestXmin (XIDs that aren't already frozen by now).
> 		 */
> 		if ((mapbits & VISIBILITYMAP_ALL_FROZEN) == 0)
> 		{
> 			if (vacrel->aggressive)
> 				break;
> 
> 			/*
> 			 * All-visible block is safe to skip in non-aggressive case.  But
> 			 * remember that the final range contains such a block for later.
> 			 */
> 			skipsallvis = true;
> 		}
> 
> 		/* XXX: is it OK to remove this? */
> 		vacuum_delay_point();
> 		next_unskippable_block++;
> 		nskippable_blocks++;
> 	}

Firstly, it seems silly to check DISABLE_PAGE_SKIPPING within the loop. 
When DISABLE_PAGE_SKIPPING is set, we always return the next block and 
set *next_unskippable_allvis = false regardless of the visibility map, 
so why bother checking the visibility map at all?

Except at the very last block of the relation! If you look carefully, 
at the last block we do return *next_unskippable_allvis = true, if the 
VM says so, even if DISABLE_PAGE_SKIPPING is set. I think that's wrong. 
Surely the intention was to pretend that none of the VM bits were set if 
DISABLE_PAGE_SKIPPING is used, also for the last block.

This was changed in commit 980ae17310:

> @@ -1311,7 +1327,11 @@ lazy_scan_skip(LVRelState *vacrel, Buffer *vmbuffer, BlockNumber next_block,
>  
>                 /* DISABLE_PAGE_SKIPPING makes all skipping unsafe */
>                 if (!vacrel->skipwithvm)
> +               {
> +                       /* Caller shouldn't rely on all_visible_according_to_vm */
> +                       *next_unskippable_allvis = false;
>                         break;
> +               }

Before that, *next_unskippable_allvis was set correctly according to the 
VM, even when DISABLE_PAGE_SKIPPING was used. It's not clear to me why 
that was changed. And I think setting it to 'true' would be a more 
failsafe value than 'false'. When *next_unskippable_allvis is set to 
true, the caller cannot rely on it because a concurrent modification 
could immediately clear the VM bit. But because VACUUM is the only 
process that sets VM bits, if it's set to false, the caller can assume 
that it's still not set later on.

One consequence of that is that with DISABLE_PAGE_SKIPPING, 
lazy_scan_heap() dirties all pages, even if there are no changes. The 
attached test script demonstrates that.

ISTM we should revert the above hunk, and backpatch it to v16. I'm a 
little wary because I don't understand why that change was made in the 
first place, though. I think it was just an ill-advised attempt at 
tidying up the code as part of the larger commit, but I'm not sure. 
Peter, do you remember?

I wonder if we should give up trying to set all_visible_according_to_vm 
correctly when we decide what to skip, and always do 
"all_visible_according_to_vm = visibilitymap_get_status(...)" in 
lazy_scan_prune(). It would be more expensive, but maybe it doesn't 
matter in practice. It would get rid of this tricky bookkeeping in 
heap_vac_scan_next_block().

-- 
Heikki Linnakangas
Neon (https://neon.tech)

--------------ZWFMzUiDj8jnB5QVuU0CAxHw
Content-Type: application/sql; name="vactest.sql"
Content-Disposition: attachment; filename="vactest.sql"
Content-Transfer-Encoding: base64

LS0gU2V0dXAKY3JlYXRlIGV4dGVuc2lvbiBpZiBub3QgZXhpc3RzIHBnX2J1ZmZlcmNhY2hl
Owpkcm9wIHRhYmxlIGlmIGV4aXN0cyB2YWN0ZXN0OwpjcmVhdGUgdGFibGUgdmFjdGVzdCAo
aWQgaW50KSB3aXRoIChhdXRvdmFjdXVtX2VuYWJsZWQ9b2ZmKTsKaW5zZXJ0IGludG8gdmFj
dGVzdCBzZWxlY3QgZyBmcm9tIGdlbmVyYXRlX3NlcmllcygxLCAxMDAwMCkgZzsKCnZhY3V1
bSBmcmVlemUgdmFjdGVzdDsKCmNoZWNrcG9pbnQ7CgotLSBBbGwgcGFnZXMgc2hvdWxkIGJl
IGFsbCBjbGVhbiBhZnRlciB0aGUgY2hlY2twb2ludC4Kc2VsZWN0IGNvdW50KCopLCBpc2Rp
cnR5IGZyb20gcGdfYnVmZmVyY2FjaGUKd2hlcmUgcmVsZmlsZW5vZGUgPSAoc2VsZWN0IHJl
bGZpbGVub2RlIGZyb20gcGdfY2xhc3Mgd2hlcmUgcmVsbmFtZT0ndmFjdGVzdCcpCmdyb3Vw
IGJ5IGlzZGlydHk7CgotLSBSdW4gdmFjdXVtIGFnYWluLiBFdmVyeXRoaW5nJ3MgYWxyZWFk
eSBmcm96ZW4sIHNvIGl0IHdpbGwganVzdCBzY2FuIHRocm91Z2gKLS0gdGFibGUgYnV0IG5v
dCBjaGFuZ2UgYW55dGhpbmcuCnZhY3V1bSAoZGlzYWJsZV9wYWdlX3NraXBwaW5nKSB2YWN0
ZXN0OwoKLS0gQWxsIHRoZSBwYWdlcyBzaG91bGQgc3RpbGwgYmUgYWxsIGNsZWFuLiBCdXQg
b24gdjE2IGFuZCBhYm92ZSwgYQotLSBsb3Qgb2YgcGFnZXMgYXJlIGFyZSBkaXJ0aWVkIQpz
ZWxlY3QgY291bnQoKiksIGlzZGlydHkgZnJvbSBwZ19idWZmZXJjYWNoZQp3aGVyZSByZWxm
aWxlbm9kZSA9IChzZWxlY3QgcmVsZmlsZW5vZGUgZnJvbSBwZ19jbGFzcyB3aGVyZSByZWxu
YW1lPSd2YWN0ZXN0JykKZ3JvdXAgYnkgaXNkaXJ0eTsKCg==

--------------ZWFMzUiDj8jnB5QVuU0CAxHw--