Thread: Indexscan API cleanup proposal

As I mentioned in passing awhile ago, I'm thinking of simplifying the
indexscan API and arranging for heapscans and indexscans to have
essentially identical APIs.  Here's the plan:

heap_beginscan and index_beginscan will have near-identical signatures:

HeapScanDesc heap_beginscan(Relation relation, Snapshot snapshot,                           int nkeys, ScanKey key)
IndexScanDesc index_beginscan(Relation relation, Snapshot snapshot,                             int nkeys, ScanKey
key)

This differs from the existing signatures in that the atend and
scanFromEnd parameters are removed --- they are useless and have only
fostered confusion.  (heap_beginscan currently ignores atend entirely
anyway.  index_beginscan looks like it does something with the parameter,
but in point of fact all the index AMs determine scan direction on the
basis of the direction first passed to index_getnext.)  Also, a snapshot
will now be passed to index_beginscan and saved in the IndexScanDesc
structure.

Similarly, the heap_rescan and index_rescan routines will drop their
scanFromEnd parameters.

The getnext routines will have the signatures

HeapTuple heap_getnext(HeapScanDesc scan, ScanDirection direction)
HeapTuple index_getnext(IndexScanDesc scan, ScanDirection direction)

with semantics in both cases essentially identical to the present
heap_getnext: return the next tuple that passes the scankeys and
snapshot tests, scanning in the specified direction; or return NULL
if no more tuples remain.  This will simplify callers of index_getnext
quite a bit.

I also intend to provide a lower-level routine

bool index_getnext_indexitem(IndexScanDesc scan, ScanDirection direction)

which finds the next index item passing the scankeys, but ignores time
qualification issues.  The actual TID of the index tuple and its
referenced heap tuple will be available from IndexScanDesc fields if
TRUE is returned.  This routine will satisfy those few callers of
index_getnext who actually wanted to see the individual index entries.
(In current code the only users of this routine will be the index AMs'
own bulk-delete routines, so I'm not sure there's any need to export
such an API at all --- but will provide it just in case.)

The interfaces from these routines to the underlying index AMs will
have parallel changes.  In particular, the index AMs' gettuple routines
will change to have signatures equivalent to index_getnext_indexitem:
rather than having to palloc and pfree an IndexRetrieveResult object
on each cycle, they'll just return TRUE or FALSE, passing the TID info
in the IndexScanDesc.

Although I think these changes are worth making just on grounds of
code beautification, the real motivation for doing this is to centralize
time-qual checking for indexscans into index_getnext, rather than having
it scattered over all the callers as at present.  Once that's done we
will have a single point of attack for addressing the problem of killing
dead index tuples in advance of VACUUM.  What I am looking to do once this
API change is in place is to make index_getnext look like
for (;;){    get next index tuple;    if (no more tuples)        return NULL;    heap_fetch corresponding heap tuple;
if (HeapTupleSatisfies(tuple, scan->snapshot))        return tuple;    /*     * If we can't see it, maybe no one can.
 */    if (HeapTupleSatisfiesVacuum(tuple) == HEAPTUPLE_DEAD)        kill the index entry;}
 

where "kill the index entry" involves informing the index AM that it can
somehow mark the index entry uninteresting and not to be returned at all
during future indexscans.  (For performance reasons this'll probably get
merged into the next "get next index tuple" operation, but that remains
to be designed in detail.)

Comments?
        regards, tom lane

Tom Lane wrote:
> where "kill the index entry" involves informing the index AM that it can
> somehow mark the index entry uninteresting and not to be returned at all
> during future indexscans.  (For performance reasons this'll probably get
> merged into the next "get next index tuple" operation, but that remains
> to be designed in detail.)
> 
> Comments?
> 

Is this a step toward being able to VACUUM indexes?

Joe

Joe Conway <mail@joeconway.com> writes:
> Tom Lane wrote:
>> where "kill the index entry" involves informing the index AM that it can
>> somehow mark the index entry uninteresting and not to be returned at all
>> during future indexscans.

> Is this a step toward being able to VACUUM indexes?

You mean collapse indexes?  No, that's an entirely different issue.
This is about reducing scan overhead when an index contains lots of
pointers to dead-but-not-yet-vacuumed tuples.
        regards, tom lane

Hello Mr. Lane,

I am a novice postgres developer. Can you please shed some light on this
problem I am having.

Basically, I am making a small change in executePlan() function of
executor/execMain.c. Right after a tupleslot is retrieved, I try to find
out the oid of the tuple that has been retrieved.

/* code that retrieves tupleslot */
/* snip */if (estate->es_useEvalPlan){            slot = EvalPlanQualNext(estate);            if (TupIsNull(slot))
             slot = ExecProcNode(plan, NULL);}else    slot = ExecProcNode(plan, NULL);
 
/* end of snip */

Right after this, I insert my code.
tupleOid = slot->val->t_data->t_oid;

For some reason, this assignment always results in tupleOid being 0. My
database has oid's enabled and I can see that an oid is assigned to each
tuple.

From what I understood, t_data is a pointer to the ondisk tuple so t_oid
should not be 0.

Can you please tell me what wrong assumptions I am making. Thank you for
your time and help.....

A budding postgresql developer,
Druv

Dhruv Pilania <dhruv@cs.sunysb.edu> writes:
> Basically, I am making a small change in executePlan() function of
> executor/execMain.c. Right after a tupleslot is retrieved, I try to find
> out the oid of the tuple that has been retrieved.

The retrieved tuple doesn't have an OID, because it's not a raw pointer
to a row on disk: it's a computed tuple (the result of ExecProject).
        regards, tom lane