Re: COPY FROM WHEN condition

On 2019-01-20 18:08:05 -0800, Andres Freund wrote:
> On 2019-01-20 21:00:21 -0500, Tomas Vondra wrote:
> > 
> > 
> > On 1/20/19 8:24 PM, Andres Freund wrote:
> > > Hi,
> > > 
> > > On 2019-01-20 00:24:05 +0100, Tomas Vondra wrote:
> > > > On 1/14/19 10:25 PM, Tomas Vondra wrote:
> > > > > On 12/13/18 8:09 AM, Surafel Temesgen wrote:
> > > > > > 
> > > > > > 
> > > > > > On Wed, Dec 12, 2018 at 9:28 PM Tomas Vondra
> > > > > > <tomas.vondra@2ndquadrant.com <mailto:tomas.vondra@2ndquadrant.com>> wrote:
> > > > > > 
> > > > > > 
> > > > > >      Can you also update the docs to mention that the functions called from
> > > > > >      the WHERE clause does not see effects of the COPY itself?
> > > > > > 
> > > > > > 
> > > > > > /Of course, i  also add same comment to insertion method selection
> > > > > > /
> > > > > 
> > > > > FWIW I've marked this as RFC and plan to get it committed this week.
> > > > > 
> > > > 
> > > > Pushed, thanks for the patch.
> > > 
> > > While rebasing the pluggable storage patch ontop of this I noticed that
> > > the qual appears to be evaluated in query context. Isn't that a bad
> > > idea? ISMT it should have been evaluated a few lines above, before the:
> > > 
> > >         /* Triggers and stuff need to be invoked in query context. */
> > >         MemoryContextSwitchTo(oldcontext);
> > > 
> > > Yes, that'd require moving the ExecStoreHeapTuple(), but that seems ok?
> > > 
> > 
> > Yes, I agree. It's a bit too late for me to hack and push stuff, but I'll
> > fix that tomorrow.
> 
> NP. On second thought, the problem is probably smaller than I thought at
> first, because ExecQual() switches to the econtext's per-tuple memory
> context. But it's only reset once for each batch, so there's some
> wastage. At least worth a comment.

I'm tired, but perhaps its actually worse - what's being reset currently
is the ESTate's per-tuple context:

        if (nBufferedTuples == 0)
        {
            /*
             * Reset the per-tuple exprcontext. We can only do this if the
             * tuple buffer is empty. (Calling the context the per-tuple
             * memory context is a bit of a misnomer now.)
             */
            ResetPerTupleExprContext(estate);
        }

but the quals are evaluated in the ExprContext's:

ExecQual(ExprState *state, ExprContext *econtext)
...
    ret = ExecEvalExprSwitchContext(state, econtext, &isnull);


which is created with:

/* Get an EState's per-output-tuple exprcontext, making it if first use */
#define GetPerTupleExprContext(estate) \
    ((estate)->es_per_tuple_exprcontext ? \
     (estate)->es_per_tuple_exprcontext : \
     MakePerTupleExprContext(estate))

and creates its own context:
    /*
     * Create working memory for expression evaluation in this context.
     */
    econtext->ecxt_per_tuple_memory =
        AllocSetContextCreate(estate->es_query_cxt,
                              "ExprContext",
                              ALLOCSET_DEFAULT_SIZES);

so this is currently just never reset. Seems just using
ExecQualAndReset() ought to be sufficient?

Greetings,

Andres Freund