Re: heapgettup() with NoMovementScanDirection unused in core?

From: Melanie Plageman <melanieplageman(at)gmail(dot)com>
To: David Rowley <dgrowleyml(at)gmail(dot)com>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: heapgettup() with NoMovementScanDirection unused in core?
Date: 2023-01-30 20:57:34
Message-ID: CAAKRu_Yg-widaxeAsUNDKK494AD6jMk2M5GUx4h841LXdAovOA@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

v2 attached

On Fri, Jan 27, 2023 at 6:28 PM David Rowley <dgrowleyml(at)gmail(dot)com> wrote:
>
> On Sat, 28 Jan 2023 at 12:15, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > /*
> > * Determine the net effect of two direction specifications.
> > * This relies on having ForwardScanDirection = +1, BackwardScanDirection = -1,
> > * and will probably not do what you want if applied to any other values.
> > */
> > #define CombineScanDirections(a, b) ((a) * (b))
> >
> > The main thing this'd buy us is being able to grep for uses of the
> > trick. If it's written as just multiplication, good luck being
> > able to find what's depending on that, should you ever need to.
>
> Yeah, I think the multiplication macro is a good way of doing it.
> Having the definition of it close to the ScanDirection enum's
> definition is likely a very good idea so that anyone adjusting the
> enum values is more likely to notice that it'll cause an issue. A
> small note on the enum declaration about the -1, +1 values being
> exploited in various places might be a good idea too. I see v6-0006 in
> [1] further exploits this, so that's further reason to document that.
>
> My personal preference would have been to call it
> ScanDirectionCombine, so the naming is more aligned to the 4 other
> macro names that start with ScanDirection in sdir.h, but I'm not going
> to fuss over it.

I've gone with this macro name.
I've also updated comments Tom mentioned and removed the test.

As for the asserts, I was at a bit of a loss as to where to put an
assert which will make it clear that heapgettup() and
heapgettup_pagemode() do not handle NoMovementScanDirection but was
at a higher level of the executor. Do we not have to accommodate the
direction changing from tuple to tuple? If we don't expect the plan node
direction to change during execution, then why recalculate
estate->es_direction for each invocation of Index/SeqNext()?

As such, in this version I've put the asserts in heapgettup() and
heapgettup_pagemode().

I also realized that it doesn't really make sense to assert about the
index scan direction in ExecInitIndexOnlyScan() and ExecInitIndexScan()
-- so I've moved the assertion to planner when we make the index plan
from the path. I'm not sure if it is needed.

- Melanie

Attachment Content-Type Size
v2-0001-remove-nomovement-scandir.patch text/x-patch 11.5 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Jacob Champion 2023-01-30 21:01:29 Re: Authentication fails for md5 connections if ~/.postgresql/postgresql.{crt and key} exist
Previous Message Bruce Momjian 2023-01-30 20:42:09 Re: run pgindent on a regular basis / scripted manner