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

<div dir="ltr"><br /><div class="gmail_extra"><br /><div class="gmail_quote">On Tue, Jan 6, 2015 at 10:51 PM, Kouhei
Kaigai<span dir="ltr"><<a href="mailto:kaigai@ak.jp.nec.com" target="_blank">kaigai@ak.jp.nec.com</a>></span>
wrote:<br/><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Jim,
Thanksfor your reviewing the patch.<br /><br /> The attached patch is revised one according to your suggestion,<br />
andalso includes bug fix I could found.<br /><br /> * Definitions of TIDXXXXOperator was moved to pg_operator.h<br />  
asother operator doing.<br /> * Support the case of "ctid (operator) Param" expression.<br /> * Add checks if
commutatorof TID was not found.<br /> * Qualifiers gets extracted on plan stage, not path stage.<br /> * Adjust cost
estimationlogic to fit SeqScan manner.<br /> * Add some new test cases for regression test.<br /><span class=""><br />
>On 12/24/14, 7:54 PM, Kouhei Kaigai wrote:<br /> > >>> On Mon, Dec 15, 2014 at 4:55 PM, Kouhei
Kaigai<br/> > >>> <<a href="mailto:kaigai@ak.jp.nec.com">kaigai@ak.jp.nec.com</a>><br /> >
>>wrote:<br /> > >>>> I'm not certain whether we should have this functionality in<br /> >
>>>>contrib from the perspective of workload that can help, but its<br /> > >>>> major worth
isfor an example of custom-scan interface.<br /> > >>> worker_spi is now in src/test/modules. We may add it
thereas well,<br /> > no?<br /> > >>><br /> > >> Hmm, it makes sense for me. Does it also make
senseto add a<br /> > >> test-case to the core regression test cases?<br /> > >><br /> > > The
attachedpatch adds ctidscan module at test/module instead of contrib.<br /> > > Basic portion is not changed from
theprevious post, but file<br /> > > locations and test cases in regression test are changed.<br /> ><br />
>First, I'm glad for this. It will be VERY valuable for anyone trying to<br /> > clean up the end of a majorly
bloatedtable.<br /> ><br /> > Here's a partial review...<br /> ><br /> > +++
b/src/test/modules/ctidscan/ctidscan.c<br/> ><br /> > +/* missing declaration in pg_proc.h */<br /> > +#ifndef
TIDGreaterOperator<br/> > +#define TIDGreaterOperator           2800<br /> > ...<br /> > If we're calling that
outhere, should we have a corresponding comment in<br /> > pg_proc.h, in case these ever get renumbered?<br />
><br/></span>It was a quick hack when I moved this module out of the tree.<br /> Yep, I should revert when I submit
thispatch again.<br /><span class=""><br /> > +CTidQualFromExpr(Node *expr, int varno)<br /> > ...<br /> > + 
          if (IsCTIDVar(arg1, varno))<br /> > +                     other = arg2;<br /> > +             else if
(IsCTIDVar(arg2,varno))<br /> > +                     other = arg1;<br /> > +             else<br /> > +     
              return NULL;<br /> > +             if (exprType(other) != TIDOID)<br /> > +                   
 returnNULL;    /* should not happen */<br /> > +             /* The other argument must be a pseudoconstant */<br
/>> +             if (!is_pseudo_constant_clause(other))<br /> > +                     return NULL;<br /> ><br
/>> I think this needs some additional blank lines...<br /> ><br /></span>OK. And, I also noticed the coding
stylearound this function is<br /> different from other built-in plans, so I redefined the role of<br /> this function
justto check whether the supplied RestrictInfo is<br /> OpExpr that involves TID inequality operator, or not.<br /><br
/>Expression node shall be extracted when Plan node is created<br /> from Path node.<br /><span class=""><br /> > + 
          if (IsCTIDVar(arg1, varno))<br /> > +                     other = arg2;<br /> > +             else if
(IsCTIDVar(arg2,varno))<br /> > +                     other = arg1;<br /> > +             else<br /> > +     
              return NULL;<br /> > +<br /> > +             if (exprType(other) != TIDOID)<br /> > +           
        return NULL;    /* should not happen */<br /> > +<br /> > +             /* The other argument must be a
pseudoconstant*/<br /> > +             if (!is_pseudo_constant_clause(other))<br /> > +                   
 returnNULL;<br /> ><br /> > + * CTidEstimateCosts<br /> > + *<br /> > + * It estimates cost to scan the
targetrelation according to the given<br /> > + * restriction clauses. Its logic to scan relations are almost same
as<br/> > + * SeqScan doing, because it uses regular heap_getnext(), except for<br /> > + * the number of tuples
tobe scanned if restriction clauses work well.<br /> ><br /> > <grammar>That should read "same as what
SeqScanis doing"... however, what<br /> > actual function are you talking about? I couldn't find
SeqScanEstimateCosts<br/> > (or anything ending EstimateCosts).<br /> ><br /></span>It is cost_seqscan(). But I
don'tput a raw function name in the source<br /> code comments in other portion, because nobody can guarantee it is up
to<br/> date in the future...<br /><span class=""><br /> > BTW, there's other grammar issues but it'd be best to
handlethose all at<br /> > once after all the code stuff is done.<br /> ><br /></span>Yep. Help by native English
speakeris very helpful for us.<br /><span class=""><br /> > +                     opno =
get_commutator(op->opno);<br/> > What happens if there's no commutator? Perhaps best to Assert(opno !=<br /> >
InvalidOid)at the end of that if block.<br /> ><br /></span>Usually, commutator operator of TID is defined on initdb
time,however,<br /> nobody can guarantee a mad superuser doesn't drop it.<br /> So, I added elog(ERROR,...) here.<br
/><spanclass=""><br /><br /> > Though, it seems things will fall apart anyway if ctid_quals isn't exactly<br /> >
whatwe're expecting; I don't know if that's OK or not.<br /> ><br /></span>No worry, it was already checked on
planningstage.<br /><span class=""><br /> > +     /* disk costs --- assume each tuple on a different page */<br />
>+     run_cost += spc_random_page_cost * ntuples;<br /> > Isn't that extremely pessimistic?<br /> ><br
/></span>Probably.I follow the manner of SeqScan.<br /><span class=""><br /> > I'm not familiar enough with the
custom-scanstuff to really comment past<br /> > this point, and I could certainly be missing some details about
planning<br/> > and execution.<br /> ><br /> > I do have some concerns about the regression test, but perhaps
I'mjust<br /> > being paranoid:<br /> ><br /> > - The EXPLAIN tests don't cover >. They do cover <= and
>=via BETWEEN.<br /> > - Nothing tests the case of '...'::tid op ctid; only lvalue cases are tested.<br />
><br/></span>Both are added.<br /><span class=""><br /> > Also, it seems that we don't handle joining on a ctid
qual...is that<br /> > intentional?<br /> ><br /></span>Yep. This module does not intend to handle joining,
becausecustom-/<br /> foreign-join interface is still under the discussion.<br />   <a
href="https://commitfest.postgresql.org/action/patch_view?id=1653"
target="_blank">https://commitfest.postgresql.org/action/patch_view?id=1653</a><br/><br /> Hanada-san makes an
enhancementof postgres_fdw on this enhancement.<br />   <a
href="https://commitfest.postgresql.org/action/patch_view?id=1674"
target="_blank">https://commitfest.postgresql.org/action/patch_view?id=1674</a><br/><span class=""><br /> > I know
thatsounds silly, but you'd probably want to do that<br /> > if you're trying to move tuples off the end of a
bloatedtable. You could<br /> > work around it by constructing a dynamic SQL string, but it'd be easier<br /> >
todo something like:<br /> ><br /> > UPDATE table1 SET ...<br /> >   WHERE ctid >= (SELECT '(' || relpages
||',0)' FROM pg_class WHERE oid<br /> > = 'table1'::regclass) ;<br /> ><br /></span>This example noticed me that
theprevious version didn't support the case<br /> of "ctid (operator) Param".<br /> So, I enhanced to support above
case,and update the regression test also.<br /></blockquote></div><br /></div><div class="gmail_extra">Moved this patch
tonext CF 2015-02 because of lack of review(ers).<br />-- <br /><div class="gmail_signature">Michael<br
/></div></div></div>