Re: heapgettup refactoring

Hi,

On 2022-10-31 14:37:39 -0400, Melanie Plageman wrote:
> and heapgetpage(). heapgettup() and heapgettup_pagemode() have a lot of
> duplicated code, confusingly nested if statements, and unnecessary local
> variables. While working on a feature for the AIO/DIO patchset, I
> noticed that it was difficult to add new code to heapgettup() and
> heapgettup_pagemode() because of how the functions are written.

Thanks for working on this - the current state is quite painful.


> From cde2d6720f4f5ab2531c22ad4a5f0d9e6ec1039d Mon Sep 17 00:00:00 2001
> From: Melanie Plageman <melanieplageman@gmail.com>
> Date: Wed, 26 Oct 2022 20:00:34 -0400
> Subject: [PATCH v1 1/3] Remove breaks in HeapTupleSatisfiesVisibility
>
> breaks in HeapTupleSatisfiesVisibility were superfluous
> ---
>  src/backend/access/heap/heapam_visibility.c | 7 -------
>  1 file changed, 7 deletions(-)
>
> diff --git a/src/backend/access/heap/heapam_visibility.c b/src/backend/access/heap/heapam_visibility.c
> index 6e33d1c881..dd5d5da190 100644
> --- a/src/backend/access/heap/heapam_visibility.c
> +++ b/src/backend/access/heap/heapam_visibility.c
> @@ -1769,25 +1769,18 @@ HeapTupleSatisfiesVisibility(HeapTuple htup, Snapshot snapshot, Buffer buffer)
>  	{
>  		case SNAPSHOT_MVCC:
>  			return HeapTupleSatisfiesMVCC(htup, snapshot, buffer);
> -			break;
>  		case SNAPSHOT_SELF:
>  			return HeapTupleSatisfiesSelf(htup, snapshot, buffer);
> -			break;
>  		case SNAPSHOT_ANY:
>  			return HeapTupleSatisfiesAny(htup, snapshot, buffer);
> -			break;
>  		case SNAPSHOT_TOAST:
>  			return HeapTupleSatisfiesToast(htup, snapshot, buffer);
> -			break;
>  		case SNAPSHOT_DIRTY:
>  			return HeapTupleSatisfiesDirty(htup, snapshot, buffer);
> -			break;
>  		case SNAPSHOT_HISTORIC_MVCC:
>  			return HeapTupleSatisfiesHistoricMVCC(htup, snapshot, buffer);
> -			break;
>  		case SNAPSHOT_NON_VACUUMABLE:
>  			return HeapTupleSatisfiesNonVacuumable(htup, snapshot, buffer);
> -			break;
>  	}

Not sure what the author of this code, a certain Mr Freund, was thinking when
he added those returns...


> From 9d8b01960463dc64ff5b111d523ff80fce3017af Mon Sep 17 00:00:00 2001
> From: Melanie Plageman <melanieplageman@gmail.com>
> Date: Mon, 31 Oct 2022 13:40:06 -0400
> Subject: [PATCH v1 2/3] Turn HeapKeyTest macro into function
>
> This should always be inlined appropriately now. It is easier to read as
> a function. Also, remove unused include in catcache.c.
> ---
>  src/backend/access/heap/heapam.c   | 10 ++--
>  src/backend/utils/cache/catcache.c |  1 -
>  src/include/access/valid.h         | 76 ++++++++++++------------------
>  3 files changed, 36 insertions(+), 51 deletions(-)
>
> diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
> index 12be87efed..1c995faa12 100644
> --- a/src/backend/access/heap/heapam.c
> +++ b/src/backend/access/heap/heapam.c
> @@ -716,8 +716,10 @@ heapgettup(HeapScanDesc scan,
>  													snapshot);
>
>  				if (valid && key != NULL)
> -					HeapKeyTest(tuple, RelationGetDescr(scan->rs_base.rs_rd),
> -								nkeys, key, valid);
> +				{
> +					valid = HeapKeyTest(tuple, RelationGetDescr(scan->rs_base.rs_rd),
> +								nkeys, key);
> +				}
>
>  				if (valid)
>  				{

superfluous parens.



> --- a/src/include/access/valid.h
> +++ b/src/include/access/valid.h
> @@ -19,51 +19,35 @@
>   *
>   *		Test a heap tuple to see if it satisfies a scan key.
>   */
> -#define HeapKeyTest(tuple, \
> -					tupdesc, \
> -					nkeys, \
> -					keys, \
> -					result) \
> -do \
> -{ \
> -	/* Use underscores to protect the variables passed in as parameters */ \
> -	int			__cur_nkeys = (nkeys); \
> -	ScanKey		__cur_keys = (keys); \
> - \
> -	(result) = true; /* may change */ \
> -	for (; __cur_nkeys--; __cur_keys++) \
> -	{ \
> -		Datum	__atp; \
> -		bool	__isnull; \
> -		Datum	__test; \
> - \
> -		if (__cur_keys->sk_flags & SK_ISNULL) \
> -		{ \
> -			(result) = false; \
> -			break; \
> -		} \
> - \
> -		__atp = heap_getattr((tuple), \
> -							 __cur_keys->sk_attno, \
> -							 (tupdesc), \
> -							 &__isnull); \
> - \
> -		if (__isnull) \
> -		{ \
> -			(result) = false; \
> -			break; \
> -		} \
> - \
> -		__test = FunctionCall2Coll(&__cur_keys->sk_func, \
> -								   __cur_keys->sk_collation, \
> -								   __atp, __cur_keys->sk_argument); \
> - \
> -		if (!DatumGetBool(__test)) \
> -		{ \
> -			(result) = false; \
> -			break; \
> -		} \
> -	} \
> -} while (0)
> +static inline bool
> +HeapKeyTest(HeapTuple tuple, TupleDesc tupdesc, int nkeys, ScanKey keys)
> +{
> +	int cur_nkeys = nkeys;
> +	ScanKey cur_key = keys;
> +
> +	for (; cur_nkeys--; cur_key++)
> +	{
> +		Datum atp;
> +		bool isnull;
> +		Datum test;
> +
> +		if (cur_key->sk_flags & SK_ISNULL)
> +			return false;
> +
> +		atp = heap_getattr(tuple, cur_key->sk_attno, tupdesc, &isnull);
> +
> +		if (isnull)
> +			return false;
> +
> +		test = FunctionCall2Coll(&cur_key->sk_func,
> +								cur_key->sk_collation,
> +								atp, cur_key->sk_argument);
> +
> +		if (!DatumGetBool(test))
> +			return false;
> +	}
> +
> +	return true;
> +}

Seems like a simple and nice win in readability.

I recall looking at this in the past and thinking that there was some
additional subtlety here, but I can't see what that'd be.



> From a894ce38c488df6546392b9f3bd894b67edf951e Mon Sep 17 00:00:00 2001
> From: Melanie Plageman <melanieplageman@gmail.com>
> Date: Mon, 31 Oct 2022 13:40:29 -0400
> Subject: [PATCH v1 3/3] Refactor heapgettup* and heapgetpage
>
> Simplify heapgettup(), heapgettup_pagemode(), and heapgetpage(). All
> three contained several unnecessary local variables, duplicate code, and
> nested if statements. Streamlining these improves readability and
> extensibility.

It'd be nice to break this into slightly smaller chunks.


> +
> +static inline void heapgettup_no_movement(HeapScanDesc scan)
> +{

FWIW, for function definitions we keep the return type (and with that also the
the "static inline") on a separate line.


> +	ItemId		lpp;
> +	OffsetNumber lineoff;
> +	BlockNumber page;
> +	Page dp;
> +	HeapTuple	tuple = &(scan->rs_ctup);
> +	/*
> +	* ``no movement'' scan direction: refetch prior tuple
> +	*/
> +
> +	/* Since the tuple was previously fetched, needn't lock page here */
> +	if (!scan->rs_inited)
> +	{
> +		Assert(!BufferIsValid(scan->rs_cbuf));
> +		tuple->t_data = NULL;
> +		return;

Is it possible to have a no-movement scan with an uninitialized scan? That
doesn't really seem to make sense. At least that's how I understand the
explanation for NoMovement nearby:
 * dir == NoMovementScanDirection means "re-fetch the tuple indicated
 * by scan->rs_ctup".

We can't have a rs_ctup without an already started scan, no?

Looks like this is pre-existing code that you just moved, but it still seems
wrong.


> +	}
> +	page = ItemPointerGetBlockNumber(&(tuple->t_self));
> +	if (page != scan->rs_cblock)
> +		heapgetpage((TableScanDesc) scan, page);


We have a
	BlockNumber page;
and
	Page		dp;
in this code which seems almost intentionally confusing. This again is a
pre-existing issue but perhaps we could clean it up first?



> +static inline Page heapgettup_continue_page(HeapScanDesc scan, BlockNumber page, ScanDirection dir,
> +		int *linesleft, OffsetNumber *lineoff)
> +{
> +	HeapTuple	tuple = &(scan->rs_ctup);

Hm. Finding the next offset via rs_ctup doesn't seem quite right. For one,
it's not actually that cheap to extract the offset from an ItemPointer because
of the the way we pack it into ItemPointerData.


> +	Page dp = BufferGetPage(scan->rs_cbuf);
> +	TestForOldSnapshot(scan->rs_base.rs_snapshot, scan->rs_base.rs_rd, dp);

Newlines between definitions and code :)

Perhaps worth asserting that the scan is initialized and that rs_cbuf is valid?


> +	if (ScanDirectionIsForward(dir))
> +	{
> +		*lineoff = OffsetNumberNext(ItemPointerGetOffsetNumber(&(tuple->t_self)));
> +		*linesleft = PageGetMaxOffsetNumber(dp) - (*lineoff) + 1;

We can't access PageGetMaxOffsetNumber etc without holding a lock on the
page. It's not immediately obvious that that is held in all paths.


> +static inline BlockNumber heapgettup_initial_page(HeapScanDesc scan, ScanDirection dir)
> +{
> +	Assert(!ScanDirectionIsNoMovement(dir));
> +	Assert(!scan->rs_inited);

Is there a reason we couldn't set rs_inited in here, rather than reapeating
that in all callers?


ISTM that this function should deal with the
			/*
			 * return null immediately if relation is empty
			 */

logic, I think you now are repeating that check on every call to heapgettup().


> @@ -511,182 +711,55 @@ heapgettup(HeapScanDesc scan,
>  		   ScanKey key)
>  {
>  	HeapTuple	tuple = &(scan->rs_ctup);
> -	Snapshot	snapshot = scan->rs_base.rs_snapshot;
> -	bool		backward = ScanDirectionIsBackward(dir);
>  	BlockNumber page;
> -	bool		finished;
>  	Page		dp;
> -	int			lines;
>  	OffsetNumber lineoff;
>  	int			linesleft;
> -	ItemId		lpp;
> +
> +	if (ScanDirectionIsNoMovement(dir))
> +		return heapgettup_no_movement(scan);

Maybe add an unlikely() - this path is barely ever used...


>  	/*
> -	 * calculate next starting lineoff, given scan direction
> +	 * return null immediately if relation is empty
>  	 */
> -	if (ScanDirectionIsForward(dir))
> +	if (scan->rs_nblocks == 0 || scan->rs_numblocks == 0)
>  	{

As mentioned above, I don't think we should repeat the nblocks check on every
call.


> +		page = scan->rs_cblock;
> +		LockBuffer(scan->rs_cbuf, BUFFER_LOCK_SHARE);
> +		dp = heapgettup_continue_page(scan, page, dir, &linesleft, &lineoff);
> +		goto continue_page;
>  	}
>
>  	/*
>  	 * advance the scan until we find a qualifying tuple or run out of stuff
>  	 * to scan
>  	 */
> -	lpp = PageGetItemId(dp, lineoff);
> -	for (;;)
> +	while (page != InvalidBlockNumber)
>  	{
> +		heapgetpage((TableScanDesc) scan, page);
> +		LockBuffer(scan->rs_cbuf, BUFFER_LOCK_SHARE);
> +		dp = heapgettup_start_page(scan, page, dir, &linesleft, &lineoff);
> +	continue_page:


I don't like the goto continue_page at all.  Seems that the paths leading here
should call LockBuffer(), heapgettup_start_page() etc?  Possibly a do {} while
() loop could do the trick as well.



Greetings,

Andres Freund