Re: Tid scan improvements

Hi,

On 2021-02-04 23:51:39 +1300, David Rowley wrote:

> I ended up adding just two new API functions to table AM.
> 
> void (*scan_set_tid_range) (TableScanDesc sscan,
>    ItemPointer mintid,
>    ItemPointer maxtid);
> 
> and
> bool (*scan_tid_range_nextslot) (TableScanDesc sscan,
> ScanDirection direction,
> TupleTableSlot *slot);
> 
> I added an additional function in tableam.h that does not have a
> corresponding API function:
> 
> static inline TableScanDesc
> table_tid_range_start(Relation rel, Snapshot snapshot,
>   ItemPointer mintid,
>   ItemPointer maxtid)
> 
> This just calls the standard scan_begin then calls scan_set_tid_range
> setting the specified mintid and maxtid.

Hm. But that means we can't rescan?


> I also added 2 new fields to TableScanDesc:
> 
> ItemPointerData rs_mintid;
> ItemPointerData rs_maxtid;
> 
> I didn't quite see a need to have a new start and end scan API function.

Yea. I guess they're not that large. Avoiding that was one of the two
reasons to have a separate scan state somewhere. The other that it
seemed like it'd possibly a bit cleaner API wise to deal with rescan.


> +bool
> +heap_getnextslot_tidrange(TableScanDesc sscan, ScanDirection direction,
> +                          TupleTableSlot *slot)
> +{
> +    HeapScanDesc scan = (HeapScanDesc) sscan;
> +    ItemPointer mintid = &sscan->rs_mintid;
> +    ItemPointer maxtid = &sscan->rs_maxtid;
> +
> +    /* Note: no locking manipulations needed */
> +    for (;;)
> +    {
> +        if (sscan->rs_flags & SO_ALLOW_PAGEMODE)
> +            heapgettup_pagemode(scan, direction, sscan->rs_nkeys, sscan->rs_key);
> +        else
> +            heapgettup(scan, direction, sscan->rs_nkeys, sscan->rs_key);
> +
> +        if (scan->rs_ctup.t_data == NULL)
> +        {
> +            ExecClearTuple(slot);
> +            return false;
> +        }
> +
> +        /*
> +         * heap_set_tidrange will have used heap_setscanlimits to limit the
> +         * range of pages we scan to only ones that can contain the TID range
> +         * we're scanning for.  Here we must filter out any tuples from these
> +         * pages that are outwith that range.
> +         */
> +        if (ItemPointerCompare(&scan->rs_ctup.t_self, mintid) < 0)
> +        {
> +            ExecClearTuple(slot);
> +
> +            /*
> +             * When scanning backwards, the TIDs will be in descending order.
> +             * Future tuples in this direction will be lower still, so we can
> +             * just return false to indicate there will be no more tuples.
> +             */
> +            if (ScanDirectionIsBackward(direction))
> +                return false;
> +
> +            continue;
> +        }
> +
> +        /*
> +         * Likewise for the final page, we must filter out TIDs greater than
> +         * maxtid.
> +         */
> +        if (ItemPointerCompare(&scan->rs_ctup.t_self, maxtid) > 0)
> +        {
> +            ExecClearTuple(slot);
> +
> +            /*
> +             * When scanning forward, the TIDs will be in ascending order.
> +             * Future tuples in this direction will be higher still, so we can
> +             * just return false to indicate there will be no more tuples.
> +             */
> +            if (ScanDirectionIsForward(direction))
> +                return false;
> +            continue;
> +        }
> +
> +        break;
> +    }
> +
> +    /*
> +     * if we get here it means we have a new current scan tuple, so point to
> +     * the proper return buffer and return the tuple.
> +     */
> +    pgstat_count_heap_getnext(scan->rs_base.rs_rd);

I wonder if there's an argument for counting the misses above via
pgstat_count_heap_fetch()? Probably not, right?


> +#define IsCTIDVar(node)  \
> +    ((node) != NULL && \
> +     IsA((node), Var) && \
> +     ((Var *) (node))->varattno == SelfItemPointerAttributeNumber && \
> +     ((Var *) (node))->varlevelsup == 0)
> +
> +typedef enum
> +{
> +    TIDEXPR_UPPER_BOUND,
> +    TIDEXPR_LOWER_BOUND
> +} TidExprType;
> +
> +/* Upper or lower range bound for scan */
> +typedef struct TidOpExpr
> +{
> +    TidExprType exprtype;        /* type of op; lower or upper */
> +    ExprState  *exprstate;        /* ExprState for a TID-yielding subexpr */
> +    bool        inclusive;        /* whether op is inclusive */
> +} TidOpExpr;
> +
> +/*
> + * For the given 'expr', build and return an appropriate TidOpExpr taking into
> + * account the expr's operator and operand order.
> + */
> +static TidOpExpr *
> +MakeTidOpExpr(OpExpr *expr, TidRangeScanState *tidstate)
> +{
> +    Node       *arg1 = get_leftop((Expr *) expr);
> +    Node       *arg2 = get_rightop((Expr *) expr);
> +    ExprState  *exprstate = NULL;
> +    bool        invert = false;
> +    TidOpExpr  *tidopexpr;
> +
> +    if (IsCTIDVar(arg1))
> +        exprstate = ExecInitExpr((Expr *) arg2, &tidstate->ss.ps);
> +    else if (IsCTIDVar(arg2))
> +    {
> +        exprstate = ExecInitExpr((Expr *) arg1, &tidstate->ss.ps);
> +        invert = true;
> +    }
> +    else
> +        elog(ERROR, "could not identify CTID variable");
> +
> +    tidopexpr = (TidOpExpr *) palloc(sizeof(TidOpExpr));
> +    tidopexpr->inclusive = false;    /* for now */
> +
> +    switch (expr->opno)
> +    {
> +        case TIDLessEqOperator:
> +            tidopexpr->inclusive = true;
> +            /* fall through */
> +        case TIDLessOperator:
> +            tidopexpr->exprtype = invert ? TIDEXPR_LOWER_BOUND : TIDEXPR_UPPER_BOUND;
> +            break;
> +        case TIDGreaterEqOperator:
> +            tidopexpr->inclusive = true;
> +            /* fall through */
> +        case TIDGreaterOperator:
> +            tidopexpr->exprtype = invert ? TIDEXPR_UPPER_BOUND : TIDEXPR_LOWER_BOUND;
> +            break;
> +        default:
> +            elog(ERROR, "could not identify CTID operator");
> +    }
> +
> +    tidopexpr->exprstate = exprstate;
> +
> +    return tidopexpr;
> +}
> +
> +/*
> + * Extract the qual subexpressions that yield TIDs to search for,
> + * and compile them into ExprStates if they're ordinary expressions.
> + */
> +static void
> +TidExprListCreate(TidRangeScanState *tidrangestate)
> +{
> +    TidRangeScan *node = (TidRangeScan *) tidrangestate->ss.ps.plan;
> +    List       *tidexprs = NIL;
> +    ListCell   *l;
> +
> +    foreach(l, node->tidrangequals)
> +    {
> +        OpExpr       *opexpr = lfirst(l);
> +        TidOpExpr  *tidopexpr;
> +
> +        if (!IsA(opexpr, OpExpr))
> +            elog(ERROR, "could not identify CTID expression");
> +
> +        tidopexpr = MakeTidOpExpr(opexpr, tidrangestate);
> +        tidexprs = lappend(tidexprs, tidopexpr);
> +    }
> +
> +    tidrangestate->trss_tidexprs = tidexprs;
> +}

Architecturally it feels like this is something that really belongs more
into plan time?


> +/*
> + * table_set_tidrange resets the minimum and maximum TID range to scan for a
> + * TableScanDesc created by table_beginscan_tidrange.
> + */
> +static inline void
> +table_set_tidrange(TableScanDesc sscan, ItemPointer mintid,
> +                   ItemPointer maxtid)
> +{
> +    /* Ensure table_beginscan_tidrange() was used. */
> +    Assert((sscan->rs_flags & SO_TYPE_TIDRANGESCAN) != 0);
> +
> +    sscan->rs_rd->rd_tableam->scan_set_tidrange(sscan, mintid, maxtid);
> +}

How does this interact with rescans?

Greetings,

Andres Freund