Re: heapgettup refactoring

On Thu, 19 Jan 2023 at 00:04, Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:
> In your v2 patch, you remove these assertions:
>
> -       /* check that rs_cindex is in sync */
> -       Assert(scan->rs_cindex < scan->rs_ntuples);
> -       Assert(lineoff == scan->rs_vistuples[scan->rs_cindex]);
>
> Is that intentional?
>
> I don't see any explanation, or some other equivalent code appearing
> elsewhere to replace this.

I guess it's because those asserts are not relevant unless
heapgettup_no_movement() is being called from heapgettup_pagemode().
Maybe they can be put back along the lines of:

Assert((scan->rs_base.rs_flags & SO_ALLOW_PAGEMODE) == 0 ||
scan->rs_cindex < scan->rs_ntuples);
Assert((scan->rs_base.rs_flags & SO_ALLOW_PAGEMODE) == 0 || lineoff ==
scan->rs_vistuples[scan->rs_cindex]);

but it probably would be cleaner to just do an: if
(scan->rs_base.rs_flags & SO_ALLOW_PAGEMODE) { Assert(...);
Assert(...}; }

The only issue I see with that is that we don't seem to have anywhere
in the regression tests that call heapgettup_no_movement() when
rs_flags have SO_ALLOW_PAGEMODE. At least, adding an elog(NOTICE) to
heapgettup() just before calling heapgettup_no_movement() does not
seem to cause make check to fail.  I wonder if any series of SQL
commands would allow us to call heapgettup_no_movement() from
heapgettup()?

I think heapgettup_no_movement() also needs a header comment more
along the lines of:

/*
 * heapgettup_no_movement
 *        Helper function for NoMovementScanDirection direction for
heapgettup() and
 *        heapgettup_pagemode.
 */

I pushed the pgindent stuff that v5-0001 did along with some additions
to typedefs.list so that further runs could be done more easily as
changes are made to these patches.

David