Review of v5:
0001: looks good.
0002:
1. I don't think you need palloc0() here. palloc() looks like it would be fine.
if (tidRangeArray->ranges == NULL)
tidRangeArray->ranges = (TidRange *)
palloc0(tidRangeArray->numAllocated * sizeof(TidRange));
if that wasn't the case, then you'll need to also zero the additional
memory when you repalloc().
2. Can't the following code be moved into the correct
forwards/backwards if block inside the if inscan block above?
/* If we've finished iterating over the ranges, exit the loop. */
if (node->tss_CurrentTidRange >= numRanges ||
node->tss_CurrentTidRange < 0)
break;
Something like:
if (bBackward)
{
if (node->tss_CurrentTidRange < 0)
{
/* initialize for backward scan */
node->tss_CurrentTidRange = numRanges - 1;
}
else if (node->tss_CurrentTidRange == 0)
break;
else
node->tss_CurrentTidRange--;
}
else
{
if (node->tss_CurrentTidRange < 0)
{
/* initialize for forward scan */
node->tss_CurrentTidRange = 0;
}
else if (node->tss_CurrentTidRange >= numRanges - 1)
break;
else
node->tss_CurrentTidRange++;
}
I think that's a few less lines and instructions and (I think) a bit neater too.
3. if (found_quals != NIL) (yeah, I Know there's already lots of
places not doing this)
/* If we found any, make an AND clause out of them. */
if (found_quals)
likewise in:
/* Use a range qual if any were found. */
if (found_quals)
4. The new tests in tidscan.sql should drop the newly created tables.
(I see some get dropped in the 0004 patch, but not all. Best not to
rely on a later patch to do work that this patch should do)
0003: looks okay.
0004:
5. Please add a comment to scandir in:
typedef struct TidScan
{
Scan scan;
List *tidquals; /* qual(s) involving CTID = something */
ScanDirection scandir;
} TidScan;
/* forward or backward or don't care */ would do.
Likewise for struct TidPath. Likely IndexPath can be used for guidance.
6. Is it worth adding a Merge Join regression test for this patch?
Something like:
postgres=# explain select * from t1 inner join t1 t2 on t1.ctid =
t2.ctid order by t1.ctid desc;
QUERY PLAN
-----------------------------------------------------------------------------
Merge Join (cost=0.00..21.25 rows=300 width=14)
Merge Cond: (t1.ctid = t2.ctid)
-> Tid Scan Backward on t1 (cost=0.00..8.00 rows=300 width=10)
-> Materialize (cost=0.00..8.75 rows=300 width=10)
-> Tid Scan Backward on t1 t2 (cost=0.00..8.00 rows=300 width=10)
(5 rows)
0005:
7. I see the logic behind this new patch, but quite possibly the
majority of the time the relpages will be out of date and you'll
mistakenly apply this to not the final page. I'm neither here nor
there with it. I imagine you might feel the same since you didn't
merge it with 0001. Maybe we can leave it out for now and see what
others think.
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
