On Tue, Jan 6, 2015 at 10:51 PM, Kouhei Kaigai <kai...@ak.jp.nec.com> wrote:
> Jim, Thanks for your reviewing the patch. > > The attached patch is revised one according to your suggestion, > and also includes bug fix I could found. > > * Definitions of TIDXXXXOperator was moved to pg_operator.h > as other operator doing. > * Support the case of "ctid (operator) Param" expression. > * Add checks if commutator of TID was not found. > * Qualifiers gets extracted on plan stage, not path stage. > * Adjust cost estimation logic to fit SeqScan manner. > * Add some new test cases for regression test. > > > On 12/24/14, 7:54 PM, Kouhei Kaigai wrote: > > >>> On Mon, Dec 15, 2014 at 4:55 PM, Kouhei Kaigai > > >>> <kai...@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? > > > It was a quick hack when I moved this module out of the tree. > Yep, I should revert when I submit this patch again. > > > +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... > > > OK. And, I also noticed the coding style around this function is > different from other built-in plans, so I redefined the role of > this function just to check whether the supplied RestrictInfo is > OpExpr that involves TID inequality operator, or not. > > Expression node shall be extracted when Plan node is created > from Path node. > > > + 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't find > SeqScanEstimateCosts > > (or anything ending EstimateCosts). > > > It is cost_seqscan(). But I don't put a raw function name in the source > code comments in other portion, because nobody can guarantee it is up to > date in the future... > > > BTW, there's other grammar issues but it'd be best to handle those all at > > once after all the code stuff is done. > > > Yep. Help by native English speaker is very helpful for us. > > > + 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. > > > Usually, commutator operator of TID is defined on initdb time, however, > nobody can guarantee a mad superuser doesn't drop it. > So, I added elog(ERROR,...) here. > > > > 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 OK or not. > > > No worry, it was already checked on planning stage. > > > + /* disk costs --- assume each tuple on a different page */ > > + run_cost += spc_random_page_cost * ntuples; > > Isn't that extremely pessimistic? > > > Probably. I follow the manner of SeqScan. > > > I'm not familiar enough with the custom-scan stuff to really comment past > > this point, and I could certainly be missing some details 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. > > > Both are added. > > > Also, it seems that we don't handle joining on a ctid qual... is that > > intentional? > > > Yep. This module does not intend to handle joining, because custom-/ > foreign-join interface is still under the discussion. > https://commitfest.postgresql.org/action/patch_view?id=1653 > > Hanada-san makes an enhancement of postgres_fdw on this enhancement. > https://commitfest.postgresql.org/action/patch_view?id=1674 > > > I know that sounds silly, but you'd probably want to do that > > if you're trying to move tuples off the end of a bloated table. You could > > work around it by constructing a 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) ; > > > This example noticed me that the previous version didn't support the case > of "ctid (operator) Param". > So, I enhanced to support above case, and update the regression test also. > Moved this patch to next CF 2015-02 because of lack of review(ers). -- Michael
