Re: ctidscan as an example of custom-scan (Re: [v9.5] Custom Plan API) - Mailing list pgsql-hackers

On 12/24/14, 7:54 PM, Kouhei Kaigai wrote:
>>> On Mon, Dec 15, 2014 at 4:55 PM, Kouhei Kaigai <kaigai@ak.jp.nec.com>
>> wrote:
>>>> I'm not certain whether we should have this functionality in contrib
>>>> from the perspective of workload that can help, but its major worth
>>>> is for an example of custom-scan interface.
>>> worker_spi is now in src/test/modules. We may add it there as well, no?
>>>
>> Hmm, it makes sense for me. Does it also make sense to add a test-case to
>> the core regression test cases?
>>
> The attached patch adds ctidscan module at test/module instead of contrib.
> Basic portion is not changed from the previous post, but file locations
> and test cases in regression test are changed.

First, I'm glad for this. It will be VERY valuable for anyone trying to clean up the end of a majorly bloated table.

Here's a partial review...

+++ b/src/test/modules/ctidscan/ctidscan.c

+/* missing declaration in pg_proc.h */
+#ifndef TIDGreaterOperator
+#define TIDGreaterOperator        2800
...
If we're calling that out here, should we have a corresponding comment in pg_proc.h, in case these ever get
renumbered?

+CTidQualFromExpr(Node *expr, int varno)
...
+        if (IsCTIDVar(arg1, varno))
+            other = arg2;
+        else if (IsCTIDVar(arg2, varno))
+            other = arg1;
+        else
+            return NULL;
+        if (exprType(other) != TIDOID)
+            return NULL;    /* should not happen */
+        /* The other argument must be a pseudoconstant */
+        if (!is_pseudo_constant_clause(other))
+            return NULL;

I think this needs some additional blank lines...

+        if (IsCTIDVar(arg1, varno))
+            other = arg2;
+        else if (IsCTIDVar(arg2, varno))
+            other = arg1;
+        else
+            return NULL;
+
+        if (exprType(other) != TIDOID)
+            return NULL;    /* should not happen */
+
+        /* The other argument must be a pseudoconstant */
+        if (!is_pseudo_constant_clause(other))
+            return NULL;

+ * CTidEstimateCosts
+ *
+ * It estimates cost to scan the target relation according to the given
+ * restriction clauses. Its logic to scan relations are almost same as
+ * SeqScan doing, because it uses regular heap_getnext(), except for
+ * the number of tuples to be scanned if restriction clauses work well.

<grammar>That should read "same as what SeqScan is doing"... however, what actual function are you talking about? I
couldn'tfind SeqScanEstimateCosts (or anything ending EstimateCosts).
 

BTW, there's other grammar issues but it'd be best to handle those all at once after all the code stuff is done.

+            opno = get_commutator(op->opno);
What happens if there's no commutator? Perhaps best to Assert(opno != InvalidOid) at the end of that if block.

Though, it seems things will fall apart anyway if ctid_quals isn't exactly what we're expecting; I don't know if that's
OKor not.
 

+    /* disk costs --- assume each tuple on a different page */
+    run_cost += spc_random_page_cost * ntuples;
Isn't that extremely pessimistic?

I'm not familiar enough with the custom-scan stuff to really comment past this point, and I could certainly be missing
somedetails about planning and execution.
 

I do have some concerns about the regression test, but perhaps I'm just being paranoid:

- The EXPLAIN tests don't cover >. They do cover <= and >= via BETWEEN.
- Nothing tests the case of '...'::tid op ctid; only lvalue cases are tested.

Also, it seems that we don't handle joining on a ctid qual... is that intentional? I know that sounds silly, but you'd
probablywant to do that if you're trying to move tuples off the end of a bloated table. You could work around it by
constructinga dynamic SQL string, but it'd be easier to do something like:
 

UPDATE table1 SET ... WHERE ctid >= (SELECT '(' || relpages || ',0)' FROM pg_class WHERE oid = 'table1'::regclass)
;

in some kind of loop.

Obviously better to only handle what you already are then not get this in at all though... :)
-- 
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com