Re: COPY FROM WHEN condition

[Andres Freund]

Hi,

On 2019-04-05 12:59:06 +1300, David Rowley wrote:
> I read through the final commit to see what had changed and I noticed
> that I had forgotten to remove nbuffers from CopyMultiInsertInfo when
> changing from a hash table to a List.
> 
> Patch to fix is attached.

Pushed, thanks.

Re: COPY FROM WHEN condition

[David Rowley]

On Fri, 5 Apr 2019 at 12:32, Andres Freund  wrote:
> I've pushed this now. Besides those naming changes, I'd to re-add the
> zero initialization (I did end up seing compiler warnings when compiling
> with optimizations).  Also some comment fixes.

Thanks for pushing.

> I added one more flush location, when inserting into a partition that
> cannot use batching - there might e.g. be triggers looking at rows in
> other partitions or such.

Good catch.

I read through the final commit to see what had changed and I noticed
that I had forgotten to remove nbuffers from CopyMultiInsertInfo when
changing from a hash table to a List.

Patch to fix is attached.

-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


remove_unused_variable_in_copy_buffers.patch
Description: Binary data

Re: COPY FROM WHEN condition

[Andres Freund]

Hi,

On 2019-04-04 12:04:28 -0700, Andres Freund wrote:
> On 2019-04-03 22:20:00 -0700, Andres Freund wrote:
> > On 2019-04-03 20:00:09 +1300, David Rowley wrote:
> > > Oops, I forgot about that one. v4 attached.
> > 
> > I'm pretty happy with this. I'm doing some minor changes (e.g. don't
> > like the function comment formatting that much, the tableam callback
> > needs docs, stuff like that), and then I'm going to push it tomorrow.
> 
> After another read, I also think I'm going to rename the functions a
> bit. CopyMultiInsertInfo_SetupBuffer, with its mix of camel-case and
> underscores, just doesn't seem to mix well with copy.c and also just
> generally other postgres code. I feel we already have too many different
> naming conventions...

I've pushed this now. Besides those naming changes, I'd to re-add the
zero initialization (I did end up seing compiler warnings when compiling
with optimizations).  Also some comment fixes.

I added one more flush location, when inserting into a partition that
cannot use batching - there might e.g. be triggers looking at rows in
other partitions or such.

I found some pretty minor slowdown for COPYing narrow rows into an
unlogged unpartitioned table. That got better by combining the loops in
CopyMultiInsertBufferFlush() (previously there were stalls due to the
indirect function calls for ExecCleartuple()). I think there's still a
tiny slowdown left in that scenario, but given that it's unlogged and
very narrow rows, I think that's ok.

Greetings,

Andres Freund

Re: COPY FROM WHEN condition

[David Rowley]

On Thu, 4 Apr 2019 at 18:20, Andres Freund  wrote:
> I'm pretty happy with this. I'm doing some minor changes (e.g. don't
> like the function comment formatting that much, the tableam callback
> needs docs, stuff like that), and then I'm going to push it tomorrow.
>
> I'm planning to attribute it to you, me, and Haribabu Kommi.

Sounds good.

>
> Oh, btw, is there a reason you're memset(0)'ing multiInsertInfo? Seems
> unnecessary, given that in all cases we're using it we're going to do
> CopyMultiInsertInfo_Init(). And IME valgrind and the compiler are more
> helpful when you don't just default initialize, because then they can
> tell when you forgot to initialize a field (say like
> CopyMultiInsertInfo_Init not initializing nbuffers).

At some point when I was hacking at it, I was getting a warning about
it being possibly uninitialised. I don't recall exactly how the code
was then, but I just tried removing it and I don't get any warning
now. So probably fine to remove.

-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: COPY FROM WHEN condition

[Andres Freund]

Hi,

On 2019-04-03 20:00:09 +1300, David Rowley wrote:
> Oops, I forgot about that one. v4 attached.

I'm pretty happy with this. I'm doing some minor changes (e.g. don't
like the function comment formatting that much, the tableam callback
needs docs, stuff like that), and then I'm going to push it tomorrow.

I'm planning to attribute it to you, me, and Haribabu Kommi.


Oh, btw, is there a reason you're memset(0)'ing multiInsertInfo? Seems
unnecessary, given that in all cases we're using it we're going to do
CopyMultiInsertInfo_Init(). And IME valgrind and the compiler are more
helpful when you don't just default initialize, because then they can
tell when you forgot to initialize a field (say like
CopyMultiInsertInfo_Init not initializing nbuffers).

Greetings,

Andres Freund

Re: COPY FROM WHEN condition

[David Rowley]

On Wed, 3 Apr 2019 at 18:56, Andres Freund  wrote:
> > +/* Class the buffer full if there are >= this many bytes of tuples stored 
> > */
> > +#define MAX_BUFFERED_BYTES   65535
>
> Random aside: This seems pretty small (but should be changed separately.

Yeah, my fingers were hovering over this. I was close to making it
1MB, but thought I'd better not.

> > +typedef struct CopyMultiInsertBuffer
> > +{
> > + TupleTableSlot *slots[MAX_BUFFERED_TUPLES]; /* Array to store tuples 
> > */
> > + ResultRelInfo *resultRelInfo;   /* ResultRelInfo for 'relid' */
> > + BulkInsertState bistate;/* BulkInsertState for this rel */
> > + int nused;  /* number of 'slots' 
> > containing tuples */
> > + uint64  linenos[MAX_BUFFERED_TUPLES];   /* Line # of tuple in 
> > copy
> > +   
> >* stream */
> > +} CopyMultiInsertBuffer;
>
> I don't think it's needed now, but you'd probably achieve a bit better
> locality by storing slots + linenos in a single array of (slot,lineno).

You're right, but right now I only zero the slots array and leave the
linenos uninitialised. Merging them to one would double the area of
memory to zero.  I'm not sure if that's worse or not, but I do know if
the number of partitions exceeds MAX_PARTITION_BUFFERS and we change
partitions quite a lot during the copy then we could end up
initialising buffers quite often. I wanted to keep that as cheap as
possible.

> > +/*
> > + * CopyMultiInsertBuffer_Init
> > + *   Allocate memory and initialize a new CopyMultiInsertBuffer 
> > for this
> > + *   ResultRelInfo.
> > + */
> > +static CopyMultiInsertBuffer *
> > +CopyMultiInsertBuffer_Init(ResultRelInfo *rri)
> > +{
> > + CopyMultiInsertBuffer *buffer;
> > +
> > + buffer = (CopyMultiInsertBuffer *) 
> > palloc(sizeof(CopyMultiInsertBuffer));
> > + memset(buffer->slots, 0, sizeof(TupleTableSlot *) * 
> > MAX_BUFFERED_TUPLES);
>
> Is there a reason to not just directly palloc0?

Yeah, I'm too cheap to zero the entire thing, per what I mentioned
above. linenos does not really need anything meaningful put there
during init. It'll get set when we consume the slots.

> > +
> > + /* Remove back-link to ourself */
> > + buffer->resultRelInfo->ri_CopyMultiInsertBuffer = NULL;
> > +
> > + ReleaseBulkInsertStatePin(buffer->bistate);
>
> Hm, afaict this still leaks the bistate itself?

Oops, I forgot about that one. v4 attached.

-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


david_tableam_copy_v4.patch
Description: Binary data

Re: COPY FROM WHEN condition

[Andres Freund]

On 2019-04-03 18:44:32 +1300, David Rowley wrote:
> (Fixed all of what you mentioned)
> 
> On Wed, 3 Apr 2019 at 06:57, Andres Freund  wrote:
> > > +/*
> > > + * CopyMultiInsertInfo_Flush
> > > + *   Write out all stored tuples in all buffers out to the 
> > > tables.
> > > + *
> > > + * To save us from ending up with buffers for 1000s of partitions we 
> > > remove
> > > + * buffers belonging to partitions that we've seen no tuples for in this 
> > > batch
> >
> > That seems a little naive (imagine you have like 5 partitions, and we
> > constantly cycle through 2-3 of them per batch).  It's probably OK for
> > this version.   I guess I'd only do that cleanup if we're actually
> > flushing due to the number of partitions.
> 
> hmm good point.  It seems like being smarter there would be a good thing.
> 
> I've ended up getting rid of the hash table in favour of the List that
> you mentioned and storing the buffer in ResultRelInfo.


Cool.

> I also changed
> the logic that removes buffers once we reach the limit.  Instead of
> getting rid of buffers that were not used on this run, I've changed it
> so it just gets rid of the buffers starting with the oldest one first,
> but stops once the number of buffers is at the maximum again.  This
> can mean that we end up with MAX_BUFFERED_TUPLES buffers instead of
> MAX_PARTITION_BUFFERS if there is only 1 tuple per buffer.  My current
> thinking is that this does not matter since only 1 slot will be
> allocated per buffer.  We'll remove all of the excess buffers during
> the flush and keep just MAX_PARTITION_BUFFERS of the newest buffers.

Yea, that makes sense to me.


> Also, after changing CopyMultiInsertBuffer to use fixed sized arrays
> instead of allocating them with another palloc the performance has
> improved a bit more.
> 
> Using the perl files mentioned in [1]
> 
> Master + Patched:
> # copy listp from program $$perl ~/bench_same.pl$$ delimiter '|';
> COPY 35651564
> Time: 9106.776 ms (00:09.107)
> # truncate table listp;
> TRUNCATE TABLE
> # copy listp from program $$perl ~/bench.pl$$ delimiter '|';
> COPY 35651564
> Time: 10154.196 ms (00:10.154)
> 
> 
> Master only:
> # copy listp from program $$perl ~/bench_same.pl$$ delimiter '|';
> COPY 35651564
> Time: 22200.535 ms (00:22.201)
> # truncate table listp;
> TRUNCATE TABLE
> # copy listp from program $$perl ~/bench.pl$$ delimiter '|';
> COPY 35651564
> Time: 18592.107 ms (00:18.592)

Awesome. I'm probably too tired to give you meaningful feedback
tonight. But I'll do a quick readthrough.



> +/* Class the buffer full if there are >= this many bytes of tuples stored */
> +#define MAX_BUFFERED_BYTES   65535

Random aside: This seems pretty small (but should be changed separately.


> +/* Trim the list of buffers back down to this number after flushing */
> +#define MAX_PARTITION_BUFFERS32
> +
> +/* Stores multi-insert data related to a single relation in CopyFrom. */
> +typedef struct CopyMultiInsertBuffer
> +{
> + TupleTableSlot *slots[MAX_BUFFERED_TUPLES]; /* Array to store tuples */
> + ResultRelInfo *resultRelInfo;   /* ResultRelInfo for 'relid' */
> + BulkInsertState bistate;/* BulkInsertState for this rel */
> + int nused;  /* number of 'slots' 
> containing tuples */
> + uint64  linenos[MAX_BUFFERED_TUPLES];   /* Line # of tuple in 
> copy
> + 
>  * stream */
> +} CopyMultiInsertBuffer;

I don't think it's needed now, but you'd probably achieve a bit better
locality by storing slots + linenos in a single array of (slot,lineno).

> +/*
> + * CopyMultiInsertBuffer_Init
> + *   Allocate memory and initialize a new CopyMultiInsertBuffer for 
> this
> + *   ResultRelInfo.
> + */
> +static CopyMultiInsertBuffer *
> +CopyMultiInsertBuffer_Init(ResultRelInfo *rri)
> +{
> + CopyMultiInsertBuffer *buffer;
> +
> + buffer = (CopyMultiInsertBuffer *) 
> palloc(sizeof(CopyMultiInsertBuffer));
> + memset(buffer->slots, 0, sizeof(TupleTableSlot *) * 
> MAX_BUFFERED_TUPLES);

Is there a reason to not just directly palloc0?


> +/*
> + * CopyMultiInsertBuffer_Cleanup
> + *   Drop used slots and free member for this buffer.  The buffer
> + *   must be flushed before cleanup.
> + */
> +static inline void
> +CopyMultiInsertBuffer_Cleanup(CopyMultiInsertBuffer *buffer)
> +{
> + int i;
> +
> + /* Ensure buffer was flushed */
> + Assert(buffer->nused == 0);
> +
> + /* Remove back-link to ourself */
> + buffer->resultRelInfo->ri_CopyMultiInsertBuffer = NULL;
> +
> + ReleaseBulkInsertStatePin(buffer->bistate);

Hm, afaict this still leaks the bistate itself?


Greetings,

Andres Freund

Re: COPY FROM WHEN condition

[David Rowley]

(Fixed all of what you mentioned)

On Wed, 3 Apr 2019 at 06:57, Andres Freund  wrote:
> > +/*
> > + * CopyMultiInsertInfo_Flush
> > + *   Write out all stored tuples in all buffers out to the tables.
> > + *
> > + * To save us from ending up with buffers for 1000s of partitions we remove
> > + * buffers belonging to partitions that we've seen no tuples for in this 
> > batch
>
> That seems a little naive (imagine you have like 5 partitions, and we
> constantly cycle through 2-3 of them per batch).  It's probably OK for
> this version.   I guess I'd only do that cleanup if we're actually
> flushing due to the number of partitions.

hmm good point.  It seems like being smarter there would be a good thing.

I've ended up getting rid of the hash table in favour of the List that
you mentioned and storing the buffer in ResultRelInfo. I also changed
the logic that removes buffers once we reach the limit.  Instead of
getting rid of buffers that were not used on this run, I've changed it
so it just gets rid of the buffers starting with the oldest one first,
but stops once the number of buffers is at the maximum again.  This
can mean that we end up with MAX_BUFFERED_TUPLES buffers instead of
MAX_PARTITION_BUFFERS if there is only 1 tuple per buffer.  My current
thinking is that this does not matter since only 1 slot will be
allocated per buffer.  We'll remove all of the excess buffers during
the flush and keep just MAX_PARTITION_BUFFERS of the newest buffers.

Also, after changing CopyMultiInsertBuffer to use fixed sized arrays
instead of allocating them with another palloc the performance has
improved a bit more.

Using the perl files mentioned in [1]

Master + Patched:
# copy listp from program $$perl ~/bench_same.pl$$ delimiter '|';
COPY 35651564
Time: 9106.776 ms (00:09.107)
# truncate table listp;
TRUNCATE TABLE
# copy listp from program $$perl ~/bench.pl$$ delimiter '|';
COPY 35651564
Time: 10154.196 ms (00:10.154)


Master only:
# copy listp from program $$perl ~/bench_same.pl$$ delimiter '|';
COPY 35651564
Time: 22200.535 ms (00:22.201)
# truncate table listp;
TRUNCATE TABLE
# copy listp from program $$perl ~/bench.pl$$ delimiter '|';
COPY 35651564
Time: 18592.107 ms (00:18.592)

> >   if (cstate->rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
> >   {
> >   ereport(ERROR,
> > - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> > - errmsg("cannot perform FREEZE on a 
> > partitioned table")));
> > + 
> > (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> > +  errmsg("cannot perform FREEZE on a 
> > partitioned table")));
> >   }
>
> accidental hunk?

It was left over from a pgindent run. Now removed.

[1] 
https://www.postgresql.org/message-id/CAKJS1f98Fa+QRTGKwqbtz0M=cy1ehyr8q-w08cpa78toy4e...@mail.gmail.com

-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


david_tableam_copy_v3.patch
Description: Binary data

Re: COPY FROM WHEN condition

[Andres Freund]

Gah, ignore this mail - I've somehow accidentally sent this early. I'm
not quite sure, but I miskeyed, and emacs sent it early, without going
through mutt...

Re: COPY FROM WHEN condition

[Andres Freund]

On 2019-04-03 06:41:49 +1300, David Rowley wrote:
> However, I've ended up not doing it that way as the patch requires
> more than just an array of TupleTableSlots to be stored in the
> ResultRelInfo, it'll pretty much need all of what I have in
> CopyMultiInsertBuffer, which includes line numbers for error
> reporting, a BulkInsertState and a counter to track how many of the
> slots are used.  I had thoughts that we could just tag the whole
> CopyMultiInsertBuffer in ResultRelInfo, but that requires moving it
> somewhere a bit more generic. Another thought would be that we have
> something like "void *extra;" in ResultRelInfo that we can just borrow
> for copy.c.  It may be interesting to try this out to see if it saves
> much in the way of performance.

Hm, we could just forwad declare struct CopyMultiInsertBuffer in a
header, and only define it in copy.c. That doesn't sound insane to me.


> I've got the patch into a working state now and wrote a bunch of
> comments. I've not really done a thorough read back of the code again.
> I normally wait until the light is coming from a different angle
> before doing that, but there does not seem to be much time to wait for
> that in this case, so here's v2.  Performance seems to be about the
> same as what I reported yesterday.

Cool.


> +/*
> + * Multi-Insert handling code for COPY FROM.  Inserts are performed in
> + * batches of up to MAX_BUFFERED_TUPLES.

This should probably be moved to the top of the file.


> + * When COPY FROM is used on a partitioned table, we attempt to maintain
> + * only MAX_PARTITION_BUFFERS buffers at once.  Buffers that are completely
> + * unused in each batch are removed so that we end up not keeping buffers for
> + * partitions that we might not insert into again.
> + */
> +#define MAX_BUFFERED_TUPLES  1000
> +#define MAX_BUFFERED_BYTES   65535
> +#define MAX_PARTITION_BUFFERS15

> +/*
> + * CopyMultiInsertBuffer
> + *   Stores multi-insert data related to a single relation in 
> CopyFrom.
> + */
> +typedef struct

Please don't create anonymous structs - they can't be forward declared,
and some debugging tools handle them worse than named structs. And it
makes naming CopyMultiInsertBuffer in the comment less necessary.


> +{
> + Oid relid;  /* Relation ID this 
> insert data is for */
> + TupleTableSlot **slots; /* Array of MAX_BUFFERED_TUPLES to store
> +  * tuples */
> + uint64 *linenos;/* Line # of tuple in copy stream */

It could make sense to allocate those two together, or even as part of
the CopyMultiInsertBuffer itself, to reduce allocator overhead.


> + else
> + {
> + CopyMultiInsertBuffer *buffer = (CopyMultiInsertBuffer *) 
> palloc(sizeof(CopyMultiInsertBuffer));
> +
> + /* Non-partitioned table.  Just setup the buffer for the table. 
> */
> + buffer->relid = RelationGetRelid(rri->ri_RelationDesc);
> + buffer->slots = palloc0(MAX_BUFFERED_TUPLES * 
> sizeof(TupleTableSlot *));
> + buffer->linenos = palloc(MAX_BUFFERED_TUPLES * sizeof(uint64));
> + buffer->resultRelInfo = rri;
> + buffer->bistate = GetBulkInsertState();
> + buffer->nused = 0;
> + miinfo->multiInsertBufferTab = NULL;
> + miinfo->buffer = buffer;
> + miinfo->nbuffers = 1;
> + }

Can this be moved into a helper function?


> + /*
> +  * heap_multi_insert leaks memory, so switch to short-lived memory 
> context
> +  * before calling it.
> +  */

s/heap_multi_insert/table_multi_insert/



> + /*
> +  * If there are any indexes, update them for all the inserted tuples, 
> and
> +  * run AFTER ROW INSERT triggers.
> +  */
> + if (resultRelInfo->ri_NumIndices > 0)
> + {
> + for (i = 0; i < nBufferedTuples; i++)
> + {
> + List   *recheckIndexes;
> +
> + cstate->cur_lineno = buffer->linenos[i];
> + recheckIndexes =
> + ExecInsertIndexTuples(buffer->slots[i], estate, 
> false, NULL,
> +   NIL);
> + ExecARInsertTriggers(estate, resultRelInfo,
> +  
> buffer->slots[i],
> +  
> recheckIndexes, cstate->transition_capture);
> + list_free(recheckIndexes);
> + }
> + }
> +
> + /*
> +  * There's no indexes, but see if we need to run AFTER ROW INSERT 
> triggers
> +  * anyway.
> +  */
> + else if (resultRelInfo->ri_TrigDesc != NULL &&
> +  (resultRelInfo->ri_TrigDesc->trig_insert_after_row ||
> +   

Re: COPY FROM WHEN condition

[Andres Freund]

On 2019-04-03 06:41:49 +1300, David Rowley wrote:
> However, I've ended up not doing it that way as the patch requires
> more than just an array of TupleTableSlots to be stored in the
> ResultRelInfo, it'll pretty much need all of what I have in
> CopyMultiInsertBuffer, which includes line numbers for error
> reporting, a BulkInsertState and a counter to track how many of the
> slots are used.  I had thoughts that we could just tag the whole
> CopyMultiInsertBuffer in ResultRelInfo, but that requires moving it
> somewhere a bit more generic. Another thought would be that we have
> something like "void *extra;" in ResultRelInfo that we can just borrow
> for copy.c.  It may be interesting to try this out to see if it saves
> much in the way of performance.

Hm, we could just forwad declare struct CopyMultiInsertBuffer in a
header, and only define it in copy.c. That doesn't sound insane to me.


> I've got the patch into a working state now and wrote a bunch of
> comments. I've not really done a thorough read back of the code again.
> I normally wait until the light is coming from a different angle
> before doing that, but there does not seem to be much time to wait for
> that in this case, so here's v2.  Performance seems to be about the
> same as what I reported yesterday.

Cool.


> +/*
> + * Multi-Insert handling code for COPY FROM.  Inserts are performed in
> + * batches of up to MAX_BUFFERED_TUPLES.

This should probably be moved to the top of the file.


> + * When COPY FROM is used on a partitioned table, we attempt to maintain
> + * only MAX_PARTITION_BUFFERS buffers at once.  Buffers that are completely
> + * unused in each batch are removed so that we end up not keeping buffers for
> + * partitions that we might not insert into again.
> + */
> +#define MAX_BUFFERED_TUPLES  1000
> +#define MAX_BUFFERED_BYTES   65535
> +#define MAX_PARTITION_BUFFERS15

> +/*
> + * CopyMultiInsertBuffer
> + *   Stores multi-insert data related to a single relation in 
> CopyFrom.
> + */
> +typedef struct

Please don't create anonymous structs - they can't be forward declared,
and some debugging tools handle them worse than named structs. And it
makes naming CopyMultiInsertBuffer in the comment less necessary.


> +{
> + Oid relid;  /* Relation ID this 
> insert data is for */
> + TupleTableSlot **slots; /* Array of MAX_BUFFERED_TUPLES to store
> +  * tuples */
> + uint64 *linenos;/* Line # of tuple in copy stream */

It could make sense to allocate those two together, or even as part of
the CopyMultiInsertBuffer itself, to reduce allocator overhead.


> + else
> + {
> + CopyMultiInsertBuffer *buffer = (CopyMultiInsertBuffer *) 
> palloc(sizeof(CopyMultiInsertBuffer));
> +
> + /* Non-partitioned table.  Just setup the buffer for the table. 
> */
> + buffer->relid = RelationGetRelid(rri->ri_RelationDesc);
> + buffer->slots = palloc0(MAX_BUFFERED_TUPLES * 
> sizeof(TupleTableSlot *));
> + buffer->linenos = palloc(MAX_BUFFERED_TUPLES * sizeof(uint64));
> + buffer->resultRelInfo = rri;
> + buffer->bistate = GetBulkInsertState();
> + buffer->nused = 0;
> + miinfo->multiInsertBufferTab = NULL;
> + miinfo->buffer = buffer;
> + miinfo->nbuffers = 1;
> + }

Can this be moved into a helper function?


> + /*
> +  * heap_multi_insert leaks memory, so switch to short-lived memory 
> context
> +  * before calling it.
> +  */

s/heap_multi_insert/table_multi_insert/



> + /*
> +  * If there are any indexes, update them for all the inserted tuples, 
> and
> +  * run AFTER ROW INSERT triggers.
> +  */
> + if (resultRelInfo->ri_NumIndices > 0)
> + {
> + for (i = 0; i < nBufferedTuples; i++)
> + {
> + List   *recheckIndexes;
> +
> + cstate->cur_lineno = buffer->linenos[i];
> + recheckIndexes =
> + ExecInsertIndexTuples(buffer->slots[i], estate, 
> false, NULL,
> +   NIL);
> + ExecARInsertTriggers(estate, resultRelInfo,
> +  
> buffer->slots[i],
> +  
> recheckIndexes, cstate->transition_capture);
> + list_free(recheckIndexes);
> + }
> + }
> +
> + /*
> +  * There's no indexes, but see if we need to run AFTER ROW INSERT 
> triggers
> +  * anyway.
> +  */
> + else if (resultRelInfo->ri_TrigDesc != NULL &&
> +  (resultRelInfo->ri_TrigDesc->trig_insert_after_row ||
> +   

Re: COPY FROM WHEN condition

[David Rowley]

On Tue, 2 Apr 2019 at 14:11, Andres Freund  wrote:
> Why do we need that list membership check? If you append the
> ResultRelInfo to the list when creating the ResultRelInfo's slots array,
> you don't need to touch the list after a partition lookup - you know
> it's a member if the ResultRelInfo has a slot array.  Then you only need
> to iterate the list when you want to drop slots to avoid using too much
> memory - and that's also a sequential scan of the hash table in your
> approach, right?

Okay, you're right, we could just determine if we've got a new
ResultRelInfo by the lack of any bulk insert slots being set in it.
However, I've ended up not doing it that way as the patch requires
more than just an array of TupleTableSlots to be stored in the
ResultRelInfo, it'll pretty much need all of what I have in
CopyMultiInsertBuffer, which includes line numbers for error
reporting, a BulkInsertState and a counter to track how many of the
slots are used.  I had thoughts that we could just tag the whole
CopyMultiInsertBuffer in ResultRelInfo, but that requires moving it
somewhere a bit more generic. Another thought would be that we have
something like "void *extra;" in ResultRelInfo that we can just borrow
for copy.c.  It may be interesting to try this out to see if it saves
much in the way of performance.

I've got the patch into a working state now and wrote a bunch of
comments. I've not really done a thorough read back of the code again.
I normally wait until the light is coming from a different angle
before doing that, but there does not seem to be much time to wait for
that in this case, so here's v2.  Performance seems to be about the
same as what I reported yesterday.

-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


david_tableam_copy_v2.patch
Description: Binary data

Re: COPY FROM WHEN condition

[Andres Freund]

Hi,

On 2019-04-02 14:06:52 +1300, David Rowley wrote:
> On Tue, 2 Apr 2019 at 13:59, Andres Freund  wrote:
> >
> > On 2019-04-02 13:41:57 +1300, David Rowley wrote:
> > > On Tue, 2 Apr 2019 at 05:19, Andres Freund  wrote:
> > > > Thanks! I'm not quite clear whether you planning to continue working on
> > > > this, or whether this is a handoff? Either is fine with me, just trying
> > > > to avoid unnecessary work / delay.
> > >
> > > I can, if you've not. I was hoping to gauge if you thought the
> > > approach was worth pursuing.
> >
> > I think it's worth pursuing, with the caveats below. I'm going to focus
> > on docs the not-very-long rest of today, but I definitely could work on
> > this afterwards. But I also would welcome any help. Let me know...
> 
> I'm looking now. I'll post something when I get it into some better
> shape than it us now.

Cool.


> > > > It still seems wrong to me to just perform a second hashtable search
> > > > here, givent that we've already done the partition dispatch.
> > >
> > > The reason I thought this was a good idea is that if we use the
> > > ResultRelInfo to buffer the tuples then there's no end to how many
> > > tuple slots can exist as the code in copy.c has no control over how
> > > many ResultRelInfos are created.
> >
> > To me those aren't contradictory - we're going to have a ResultRelInfo
> > for each partition either way, but there's nothing preventing copy.c
> > from cleaning up subsidiary data in it.  What I was thinking is that
> > we'd just keep track of a list of ResultRelInfos with bulk insert slots,
> > and occasionally clean them up. That way we avoid the secondary lookup,
> > while also managing the amount of slots.
> 
> The problem that I see with that is you can't just add to that list
> when the partition changes. You must check if the ResultRelInfo is
> already in the list or not since we could change partitions and change
> back again. For a list with just a few elements checking
> list_member_ptr should be pretty cheap, but I randomly did choose that
> we try to keep just the last 16 partitions worth of buffers. I don't
> think checking list_member_ptr in a 16 element list is likely to be
> faster than a hash table lookup, do you?

Why do we need that list membership check? If you append the
ResultRelInfo to the list when creating the ResultRelInfo's slots array,
you don't need to touch the list after a partition lookup - you know
it's a member if the ResultRelInfo has a slot array.  Then you only need
to iterate the list when you want to drop slots to avoid using too much
memory - and that's also a sequential scan of the hash table in your
approach, right?

Greetings,

Andres Freund

Re: COPY FROM WHEN condition

[David Rowley]

On Tue, 2 Apr 2019 at 13:59, Andres Freund  wrote:
>
> On 2019-04-02 13:41:57 +1300, David Rowley wrote:
> > On Tue, 2 Apr 2019 at 05:19, Andres Freund  wrote:
> > > Thanks! I'm not quite clear whether you planning to continue working on
> > > this, or whether this is a handoff? Either is fine with me, just trying
> > > to avoid unnecessary work / delay.
> >
> > I can, if you've not. I was hoping to gauge if you thought the
> > approach was worth pursuing.
>
> I think it's worth pursuing, with the caveats below. I'm going to focus
> on docs the not-very-long rest of today, but I definitely could work on
> this afterwards. But I also would welcome any help. Let me know...

I'm looking now. I'll post something when I get it into some better
shape than it us now.

> > > It still seems wrong to me to just perform a second hashtable search
> > > here, givent that we've already done the partition dispatch.
> >
> > The reason I thought this was a good idea is that if we use the
> > ResultRelInfo to buffer the tuples then there's no end to how many
> > tuple slots can exist as the code in copy.c has no control over how
> > many ResultRelInfos are created.
>
> To me those aren't contradictory - we're going to have a ResultRelInfo
> for each partition either way, but there's nothing preventing copy.c
> from cleaning up subsidiary data in it.  What I was thinking is that
> we'd just keep track of a list of ResultRelInfos with bulk insert slots,
> and occasionally clean them up. That way we avoid the secondary lookup,
> while also managing the amount of slots.

The problem that I see with that is you can't just add to that list
when the partition changes. You must check if the ResultRelInfo is
already in the list or not since we could change partitions and change
back again. For a list with just a few elements checking
list_member_ptr should be pretty cheap, but I randomly did choose that
we try to keep just the last 16 partitions worth of buffers. I don't
think checking list_member_ptr in a 16 element list is likely to be
faster than a hash table lookup, do you?

-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: COPY FROM WHEN condition

[Andres Freund]

Hi,

On 2019-04-02 13:41:57 +1300, David Rowley wrote:
> On Tue, 2 Apr 2019 at 05:19, Andres Freund  wrote:
> >
> > On 2019-04-02 04:23:20 +1300, David Rowley wrote:
> > > On Mon, 1 Apr 2019 at 08:05, Andres Freund  wrote:
> > > > I'll work on pushing all the other pending tableam patches today -
> > > > leaving COPY the last non pluggable part. You'd written in a private
> > > > email that you might try to work on this on Monday, so I think I'll give
> > > > this a shot on Tuesday if you've not gotten around till then? I'd like
> > > > to push this sooner than the exact end of the freeze...
> > >
> > > I worked on this, but I've not got the patch finished yet.
> >
> > Thanks! I'm not quite clear whether you planning to continue working on
> > this, or whether this is a handoff? Either is fine with me, just trying
> > to avoid unnecessary work / delay.
> 
> I can, if you've not. I was hoping to gauge if you thought the
> approach was worth pursuing.

I think it's worth pursuing, with the caveats below. I'm going to focus
on docs the not-very-long rest of today, but I definitely could work on
this afterwards. But I also would welcome any help. Let me know...



> > > +static inline void
> > > +CopyMultiInsertBuffer_RemoveBuffer(CopyMultiInsertInfo *miinfo, 
> > > CopyMultiInsertBuffer *buffer)
> > > +{
> > > + Oid relid = buffer->relid;
> > > + int i = 0;
> > > +
> > > + while (buffer->slots[i] != NULL)
> > > + ExecClearTuple(buffer->slots[i++]);
> > > + pfree(buffer->slots);
> > > +
> > > + hash_search(miinfo->multiInsertBufferTab, (void *) , 
> > > HASH_REMOVE,
> > > + NULL);
> > > + miinfo->nbuffers--;
> > > +}
> >
> > Hm, this leaves dangling slots, right?
> 
> Probably.  I didn't quite finish figuring out how a slot should have
> all its memory released.

ExecDropSingleTupleTableSlot() ought to do the job (if not added to a
tuptable/estate - which we shouldn't as currently one-by-one removal
from therein is expensive).


> > It still seems wrong to me to just perform a second hashtable search
> > here, givent that we've already done the partition dispatch.
> 
> The reason I thought this was a good idea is that if we use the
> ResultRelInfo to buffer the tuples then there's no end to how many
> tuple slots can exist as the code in copy.c has no control over how
> many ResultRelInfos are created.

To me those aren't contradictory - we're going to have a ResultRelInfo
for each partition either way, but there's nothing preventing copy.c
from cleaning up subsidiary data in it.  What I was thinking is that
we'd just keep track of a list of ResultRelInfos with bulk insert slots,
and occasionally clean them up. That way we avoid the secondary lookup,
while also managing the amount of slots.


Greetings,

Andres Freund

Re: COPY FROM WHEN condition

[David Rowley]

On Tue, 2 Apr 2019 at 05:19, Andres Freund  wrote:
>
> On 2019-04-02 04:23:20 +1300, David Rowley wrote:
> > On Mon, 1 Apr 2019 at 08:05, Andres Freund  wrote:
> > > I'll work on pushing all the other pending tableam patches today -
> > > leaving COPY the last non pluggable part. You'd written in a private
> > > email that you might try to work on this on Monday, so I think I'll give
> > > this a shot on Tuesday if you've not gotten around till then? I'd like
> > > to push this sooner than the exact end of the freeze...
> >
> > I worked on this, but I've not got the patch finished yet.
>
> Thanks! I'm not quite clear whether you planning to continue working on
> this, or whether this is a handoff? Either is fine with me, just trying
> to avoid unnecessary work / delay.

I can, if you've not. I was hoping to gauge if you thought the
approach was worth pursuing.

> > Of course, it is possible that some of the bugs account for some of
> > the improved time...  but the rows did seem to be in the table
> > afterwards.
>
> The speedup likely seems to be from having much larger batches, right?
> Unless we insert into enough partitions that batching doesn't ever
> encounter two tuples, we'll now efficiently use multi_insert even if
> there's no consecutive tuples.

I imagine that's part of it, but I was surprised that the test that
inserts into the same partition also was improved fairly
significantly.

> > +static inline void
> > +CopyMultiInsertBuffer_RemoveBuffer(CopyMultiInsertInfo *miinfo, 
> > CopyMultiInsertBuffer *buffer)
> > +{
> > + Oid relid = buffer->relid;
> > + int i = 0;
> > +
> > + while (buffer->slots[i] != NULL)
> > + ExecClearTuple(buffer->slots[i++]);
> > + pfree(buffer->slots);
> > +
> > + hash_search(miinfo->multiInsertBufferTab, (void *) , 
> > HASH_REMOVE,
> > + NULL);
> > + miinfo->nbuffers--;
> > +}
>
> Hm, this leaves dangling slots, right?

Probably.  I didn't quite finish figuring out how a slot should have
all its memory released.

> It still seems wrong to me to just perform a second hashtable search
> here, givent that we've already done the partition dispatch.

The reason I thought this was a good idea is that if we use the
ResultRelInfo to buffer the tuples then there's no end to how many
tuple slots can exist as the code in copy.c has no control over how
many ResultRelInfos are created.

> >   if (proute)
> >   insertMethod = CIM_MULTI_CONDITIONAL;
> >   else
> >   insertMethod = CIM_MULTI;
>
> Hm, why do we still have CIM_MULTI and CIM_MULTI_CONDITIONAL?

We don't know what partitions will be foreign or have triggers that
don't allow us to multi-insert. Multi-inserts are still conditional
based on that.

> Not for now / this commit, but I kinda feel like we should determine
> whether it's safe to multi-insert at a more centralized location.

Yeah, I was thinking that might be nice but teaching those
Multi-insert functions about that means they'd need to handle
non-multi inserts too. That might only be a problem that highlights
that the functions I added are not named very well.

-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: COPY FROM WHEN condition

[Andres Freund]

Hi,

On 2019-04-02 04:23:20 +1300, David Rowley wrote:
> On Mon, 1 Apr 2019 at 08:05, Andres Freund  wrote:
> > I'll work on pushing all the other pending tableam patches today -
> > leaving COPY the last non pluggable part. You'd written in a private
> > email that you might try to work on this on Monday, so I think I'll give
> > this a shot on Tuesday if you've not gotten around till then? I'd like
> > to push this sooner than the exact end of the freeze...
> 
> I worked on this, but I've not got the patch finished yet.

Thanks! I'm not quite clear whether you planning to continue working on
this, or whether this is a handoff? Either is fine with me, just trying
to avoid unnecessary work / delay.


> Of course, it is possible that some of the bugs account for some of
> the improved time...  but the rows did seem to be in the table
> afterwards.

The speedup likely seems to be from having much larger batches, right?
Unless we insert into enough partitions that batching doesn't ever
encounter two tuples, we'll now efficiently use multi_insert even if
there's no consecutive tuples.


>   if (cstate->rel)
>   {
> - Datum  *values;
> - bool   *nulls;
> + TupleTableSlot *slot;
>   TableScanDesc scandesc;
> - HeapTuple   tuple;
> -
> - values = (Datum *) palloc(num_phys_attrs * sizeof(Datum));
> - nulls = (bool *) palloc(num_phys_attrs * sizeof(bool));
>  
>   scandesc = table_beginscan(cstate->rel, GetActiveSnapshot(), 0, 
> NULL);
> + slot = table_slot_create(cstate->rel, NULL);
>  
>   processed = 0;
> - while ((tuple = heap_getnext(scandesc, ForwardScanDirection)) 
> != NULL)
> + while (table_scan_getnextslot(scandesc, ForwardScanDirection, 
> slot))
>   {
>   CHECK_FOR_INTERRUPTS();
>  
> - /* Deconstruct the tuple ... faster than repeated 
> heap_getattr */
> - heap_deform_tuple(tuple, tupDesc, values, nulls);
> + /* Deconstruct the tuple ... */
> + slot_getallattrs(slot);
>  
>   /* Format and send the data */
> - CopyOneRowTo(cstate, values, nulls);
> + CopyOneRowTo(cstate, slot);
>   processed++;
>   }
>  
> + ExecDropSingleTupleTableSlot(slot);
>   table_endscan(scandesc);
> -
> - pfree(values);
> - pfree(nulls);
>   }

Hm, I should just have committed this part separately. Not sure why I
didn't...


> +#define MAX_BUFFERED_TUPLES  1000
> +#define MAX_BUFFERED_BYTES   65535
> +#define MAX_PARTITION_BUFFERS16
> +
> +typedef struct {
> + Oid relid;
> + TupleTableSlot **slots;
> + ResultRelInfo *resultRelInfo;
> + int nused;
> +} CopyMultiInsertBuffer;
> +
> +typedef struct {
> + HTAB *multiInsertBufferTab;
> + CopyMultiInsertBuffer *buffer;
> + int bufferedTuples;
> + int bufferedBytes;
> + int nbuffers;
> +} CopyMultiInsertInfo;

To me it seems a bit better to have this as generic facility - we really
should use multi_insert in more places (like inserting the pg_attribute
rows). But admittedly that can just be done later.


> +static inline void
> +CopyMultiInsertBuffer_RemoveBuffer(CopyMultiInsertInfo *miinfo, 
> CopyMultiInsertBuffer *buffer)
> +{
> + Oid relid = buffer->relid;
> + int i = 0;
> +
> + while (buffer->slots[i] != NULL)
> + ExecClearTuple(buffer->slots[i++]);
> + pfree(buffer->slots);
> +
> + hash_search(miinfo->multiInsertBufferTab, (void *) , HASH_REMOVE,
> + NULL);
> + miinfo->nbuffers--;
> +}

Hm, this leaves dangling slots, right?



> +static inline void
> +CopyMultiInsertInfo_SetCurrentBuffer(CopyMultiInsertInfo *miinfo,
> +  
> ResultRelInfo *rri)
> +{
> + CopyMultiInsertBuffer *buffer;
> + Oid relid = RelationGetRelid(rri->ri_RelationDesc);
> + boolfound;
> +
> + Assert(miinfo->multiInsertBufferTab != NULL);
> +
> + buffer = hash_search(miinfo->multiInsertBufferTab, , HASH_ENTER, 
> );
> +
> + if (!found)
> + {
> + buffer->relid = relid;
> + buffer->slots = palloc0(MAX_BUFFERED_TUPLES * 
> sizeof(TupleTableSlot *));
> + buffer->resultRelInfo = rri;
> + buffer->nused = 0;
> + miinfo->nbuffers++;
> + }
> +
> + miinfo->buffer = buffer;
> +}

It still seems wrong to me to just perform a second hashtable search
here, givent that we've already done the partition dispatch.



>   if (proute)
>   insertMethod = CIM_MULTI_CONDITIONAL;
>   else
>   insertMethod = CIM_MULTI;

Re: COPY FROM WHEN condition

[David Rowley]

On Mon, 1 Apr 2019 at 08:05, Andres Freund  wrote:
> I'll work on pushing all the other pending tableam patches today -
> leaving COPY the last non pluggable part. You'd written in a private
> email that you might try to work on this on Monday, so I think I'll give
> this a shot on Tuesday if you've not gotten around till then? I'd like
> to push this sooner than the exact end of the freeze...

I worked on this, but I've not got the patch finished yet.

Things not done:

1. Fails regression tests. Possibly due to a bug in tuple conversion.
2. Thought about ERROR line numbers in copy stream. It's possible we
may need an array of uint64s to store the line number per slot.
3. Testing.
4. Code comments.
5. Code sanity check.
6. Thought about if it's a problem if we ERROR during the copy after
having already inserted tuples that come after the tuple causing the
error. (It's possible that the errors would become out of order.)

However, the performance looks pretty good.

$ cat bench.pl
for (my $i=0; $i < 8912891; $i++) {
print "1\n1\n2\n2\n";
}
$ cat bench_same.pl
for (my $i=0; $i < 8912891; $i++) {
print "1\n1\n1\n1\n";
}

create table listp(a int) partition by list(a);
create table listp1 partition of listp for values in(1);
create table listp2 partition of listp for values in(2);

-- Test 1: Change partition every 2nd tuple.

master + v23-0001-tableam-multi_insert-and-slotify-COPY.patch

# copy listp from program $$perl ~/bench.pl$$ delimiter '|';
Time: 17894.625 ms (00:17.895)

master + attached

# copy listp from program $$perl ~/bench.pl$$ delimiter '|';
Time: 10615.761 ms (00:10.616)

-- Test 2: Same partition each time.

master + v23-0001-tableam-multi_insert-and-slotify-COPY.patch
# copy listp from program $$perl ~/bench_same.pl$$ delimiter '|';
Time: 19234.960 ms (00:19.235)

master + attached

# copy listp from program $$perl ~/bench_same.pl$$ delimiter '|';
Time: 9064.802 ms (00:09.065)

Of course, it is possible that some of the bugs account for some of
the improved time...  but the rows did seem to be in the table
afterwards.

-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


wip_tableam_copy.patch
Description: Binary data

Re: COPY FROM WHEN condition

[Andres Freund]

Hi,

On 2019-04-01 02:00:26 +1300, David Rowley wrote:
> On Fri, 29 Mar 2019 at 01:15, Andres Freund  wrote:
> > On 2019-03-28 20:48:47 +1300, David Rowley wrote:
> > > I had a look at this and performance has improved again, thanks.
> > > However, I'm not sure if the patch is exactly what we need, let me
> > > explain.
> >
> > I'm not entirely sure either, I just haven't really seen an alternative
> > that's convincing.
> 
> I wonder if instead of having the array of slots in ResultRelInfo,
> have a struct that's local to copy.c containing the array and the
> number of tuples stored so far.  For partitioned tables, we could
> store this struct in a hashtable by partition Oid. When the partition
> changes check if we've got this partition Oid in the hash table and
> keep adding tuples until the buffer fills.   We could keep a global
> count of the number of tuple stored in all the slot arrays and flush
> all of them when it gets full.
> 
> The trade-off here would be that instead of flushing on each partition
> change, we'd do a hash table lookup on each partition change and
> possibly create a new array of slots.   This would allow us to get rid
> of the code that conditionally switches on/off the batching based on
> how often the partition is changing. The key to it being better would
> hang on the hash lookup + multi-row-inserts being faster than
> single-row-inserts.

It's not clear to me why this separate hashtable is useful /
necessary. We're essentially already doing such a lookup for the
partition routing - what's the point of having a repetition of the same
mapping?  I don't see any benefit of storing the slots separately from
that.


> I'm just not too sure about how to handle getting rid of the slots
> when we flush all the tuples.  Getting rid of them might be a waste,
> but it might also stop the code creating tens of millions of slots in
> the worst case.  Maybe to fix that we could get rid of the slots in
> arrays that didn't get any use at all when we flush the tuples, as
> indicated by a 0 tuple count.  This would require a hash seq scan, but
> maybe we could keep that cheap by flushing early if we get too many
> distinct partitions.

I'd suspect a better approach to this might be to just have a linked
list of partitions with slots in them, that'd avoid unnecessarily going
through all the partitions that got copied into them.  I'm not convinced
this is necessary, but if I were to implement this, I'd leave the slots
in the ResultRelInfo, but additionally keep track of how many slots have
been created. When creating new slots, and some limit has been reached,
I'd just go to the front of that list and delete those slots (probably
delaying doing so to the the point of flushing pending insertions).
That seems like it'd not actually be that much changed code over my
current patch, and would avoid deleting slots in the common case.

I'll work on pushing all the other pending tableam patches today -
leaving COPY the last non pluggable part. You'd written in a private
email that you might try to work on this on Monday, so I think I'll give
this a shot on Tuesday if you've not gotten around till then? I'd like
to push this sooner than the exact end of the freeze...


> That would save the table from getting bloated if
> there happened to be a point in the copy stream where we saw high
> numbers of distinct partitions with just a few tuples each.
> Multi-inserts won't help much in that case anyway.

I thought your benchmarking showed that you saw benefits after even two
tuples?

Greetings,

Andres Freund

Re: COPY FROM WHEN condition

[David Rowley]

On Fri, 29 Mar 2019 at 01:15, Andres Freund  wrote:
> On 2019-03-28 20:48:47 +1300, David Rowley wrote:
> > I had a look at this and performance has improved again, thanks.
> > However, I'm not sure if the patch is exactly what we need, let me
> > explain.
>
> I'm not entirely sure either, I just haven't really seen an alternative
> that's convincing.

I wonder if instead of having the array of slots in ResultRelInfo,
have a struct that's local to copy.c containing the array and the
number of tuples stored so far.  For partitioned tables, we could
store this struct in a hashtable by partition Oid. When the partition
changes check if we've got this partition Oid in the hash table and
keep adding tuples until the buffer fills.   We could keep a global
count of the number of tuple stored in all the slot arrays and flush
all of them when it gets full.

The trade-off here would be that instead of flushing on each partition
change, we'd do a hash table lookup on each partition change and
possibly create a new array of slots.   This would allow us to get rid
of the code that conditionally switches on/off the batching based on
how often the partition is changing. The key to it being better would
hang on the hash lookup + multi-row-inserts being faster than
single-row-inserts.

I'm just not too sure about how to handle getting rid of the slots
when we flush all the tuples.  Getting rid of them might be a waste,
but it might also stop the code creating tens of millions of slots in
the worst case.  Maybe to fix that we could get rid of the slots in
arrays that didn't get any use at all when we flush the tuples, as
indicated by a 0 tuple count.  This would require a hash seq scan, but
maybe we could keep that cheap by flushing early if we get too many
distinct partitions. That would save the table from getting bloated if
there happened to be a point in the copy stream where we saw high
numbers of distinct partitions with just a few tuples each.
Multi-inserts won't help much in that case anyway.

-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: COPY FROM WHEN condition

[Andres Freund]

Hi,

On 2019-03-28 20:48:47 +1300, David Rowley wrote:
> I had a look at this and performance has improved again, thanks.
> However, I'm not sure if the patch is exactly what we need, let me
> explain.

I'm not entirely sure either, I just haven't really seen an alternative
that's convincing.


> When I was working on 0d5f05cde01, I think my original idea was just
> to use the bufferedTuples[] array and always multi insert into
> partitioned tables unless something like on insert triggers stopped
> that.  The reason I ended up with all this CIM_MULTI and
> CIM_MULTI_CONDITIONAL stuff is due to Melanie finding a regression
> when the target partition changed on each tuple.  At the time I
> wondered if it might be worth trying to have multiple buffers or to
> partition the existing buffer and store tuples belonging to different
> partitions, then just flush them when they're full.  Looking at the
> patch it seems that you have added an array of slots per partition, so
> does that not kinda make all that CIM_MULTI / CIM_MULTI_CONDITIONAL
> code redundant? ... we could just flush each partition's buffer when
> it gets full.  There's not much of a need to flush the buffer when the
> partition changes since we're using a different buffer for the next
> partition anyway.

Well, there is in a way - that'd lead to pretty significant memory
usage if the tuples are a bit wider.  I think there's a separate
optimization, where we keep track of the partitions with unflushed
changes and track the buffer size across all partitions. But that seems
like a followup patch.


> I also wonder about memory usage here. If we're copying to a
> partitioned table with 10k partitions and we get over
> MAX_BUFFERED_TUPLES in a row for each partition, we'll end up with
> quite a lot of slots.

We do - but they're not actually that large. In comparison to the
catcache, relcache, partition routing etc information per partition it's
not a huge amount of memory.

I'd earlier just destroyed the slots on partition change, but that's not
exactly free either...


Greetings,

Andres Freund

Re: COPY FROM WHEN condition

[David Rowley]

On Wed, 27 Mar 2019 at 18:49, Andres Freund  wrote:
> Here's a version that applies onto HEAD. It needs a bit more cleanup
> (I'm not sure I like the addition of ri_batchInsertSlots to store slots
> for each partition, there's some duplicated code around slot array and
> slot creation, docs for the multi insert callback).
>
> When measuring inserting into unlogged partitions (to remove the
> overhead of WAL logging, which makes it harder to see the raw
> performance difference), I see a few percent gains of the slotified
> version over the current code.  Same with insertions that have fewer
> partition switches.
>
> Sorry for posting this here - David, it'd be cool if you could take a
> look anyway. You've played a lot with this code.

Hi,

I had a look at this and performance has improved again, thanks.
However, I'm not sure if the patch is exactly what we need, let me
explain.

When I was working on 0d5f05cde01, I think my original idea was just
to use the bufferedTuples[] array and always multi insert into
partitioned tables unless something like on insert triggers stopped
that.  The reason I ended up with all this CIM_MULTI and
CIM_MULTI_CONDITIONAL stuff is due to Melanie finding a regression
when the target partition changed on each tuple.  At the time I
wondered if it might be worth trying to have multiple buffers or to
partition the existing buffer and store tuples belonging to different
partitions, then just flush them when they're full.  Looking at the
patch it seems that you have added an array of slots per partition, so
does that not kinda make all that CIM_MULTI / CIM_MULTI_CONDITIONAL
code redundant? ... we could just flush each partition's buffer when
it gets full.  There's not much of a need to flush the buffer when the
partition changes since we're using a different buffer for the next
partition anyway.

I also wonder about memory usage here. If we're copying to a
partitioned table with 10k partitions and we get over
MAX_BUFFERED_TUPLES in a row for each partition, we'll end up with
quite a lot of slots.

So, in short. I think the patch either is going to cause possible
memory problems during COPY FROM, or it leaves code that is pretty
much redundant. ... Or I've just misunderstood the patch :)

Does this make sense?

-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: COPY FROM WHEN condition

[Andres Freund]

Hi,

On 2019-01-29 07:22:16 -0800, Andres Freund wrote:
> On January 29, 2019 6:25:59 AM PST, Tomas Vondra 
>  wrote:
> >On 1/29/19 8:18 AM, David Rowley wrote:
> >> ...
> >> Here are my performance tests of with and without your change to the
> >> memory contexts (I missed where you posted your results).
> >> 
> >> $ cat bench.pl
> >> for (my $i=0; $i < 8912891; $i++) {
> >> print "1\n1\n2\n2\n";
> >> }
> >> 36a1281f86c: (with your change)
> >> 
> >> postgres=# copy listp from program $$perl ~/bench.pl$$ delimiter '|';
> >> COPY 35651564
> >> Time: 26825.142 ms (00:26.825)
> >> Time: 27202.117 ms (00:27.202)
> >> Time: 26266.705 ms (00:26.267)
> >> 
> >> 4be058fe9ec: (before your change)
> >> 
> >> postgres=# copy listp from program $$perl ~/bench.pl$$ delimiter '|';
> >> COPY 35651564
> >> Time: 25645.460 ms (00:25.645)
> >> Time: 25698.193 ms (00:25.698)
> >> Time: 25737.251 ms (00:25.737)
> >> 
> >
> >How do I reproduce this? I don't see this test in the thread you
> >linked,
> >so I'm not sure how many partitions you were using, what's the
> >structure
> >of the table etc.
> 
> I think I might have a patch addressing the problem incidentally. For
> pluggable storage I slotified copy.c, which also removes the first
> heap_form_tuple. Quite possible that nothing more is needed. I've
> removed the batch context altogether in yesterday's rebase, there was
> no need anymore.

Here's a version that applies onto HEAD. It needs a bit more cleanup
(I'm not sure I like the addition of ri_batchInsertSlots to store slots
for each partition, there's some duplicated code around slot array and
slot creation, docs for the multi insert callback).

When measuring inserting into unlogged partitions (to remove the
overhead of WAL logging, which makes it harder to see the raw
performance difference), I see a few percent gains of the slotified
version over the current code.  Same with insertions that have fewer
partition switches.

Sorry for posting this here - David, it'd be cool if you could take a
look anyway. You've played a lot with this code.

Btw, for v13, I hope that either I or somebody else cleans up CopyFrom()
a bit - it's gotten really hard to understand. I think we need to split
it into a few pieces.

Greetings,

Andres Freund
>From 3e8e43232b08eac40bb5f634be158b8577fc844f Mon Sep 17 00:00:00 2001
From: Andres Freund 
Date: Sun, 20 Jan 2019 13:28:26 -0800
Subject: [PATCH v23] tableam: multi_insert and slotify COPY.

Author:
Reviewed-By:
Discussion: https://postgr.es/m/
Backpatch:
---
 src/backend/access/heap/heapam.c |  23 +-
 src/backend/access/heap/heapam_handler.c |   1 +
 src/backend/commands/copy.c  | 322 ---
 src/include/access/heapam.h  |   3 +-
 src/include/access/tableam.h |  14 +
 src/include/nodes/execnodes.h|   6 +
 6 files changed, 204 insertions(+), 165 deletions(-)

diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index f3812dd5871..71d14789c98 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -2106,7 +2106,7 @@ heap_prepare_insert(Relation relation, HeapTuple tup, TransactionId xid,
  * temporary context before calling this, if that's a problem.
  */
 void
-heap_multi_insert(Relation relation, HeapTuple *tuples, int ntuples,
+heap_multi_insert(Relation relation, TupleTableSlot **slots, int ntuples,
   CommandId cid, int options, BulkInsertState bistate)
 {
 	TransactionId xid = GetCurrentTransactionId();
@@ -2127,11 +2127,18 @@ heap_multi_insert(Relation relation, HeapTuple *tuples, int ntuples,
 	saveFreeSpace = RelationGetTargetPageFreeSpace(relation,
    HEAP_DEFAULT_FILLFACTOR);
 
-	/* Toast and set header data in all the tuples */
+	/* Toast and set header data in all the slots */
 	heaptuples = palloc(ntuples * sizeof(HeapTuple));
 	for (i = 0; i < ntuples; i++)
-		heaptuples[i] = heap_prepare_insert(relation, tuples[i],
-			xid, cid, options);
+	{
+		HeapTuple tuple;
+
+		tuple = ExecFetchSlotHeapTuple(slots[i], true, NULL);
+		slots[i]->tts_tableOid = RelationGetRelid(relation);
+		tuple->t_tableOid = slots[i]->tts_tableOid;
+		heaptuples[i] = heap_prepare_insert(relation, tuple, xid, cid,
+			options);
+	}
 
 	/*
 	 * We're about to do the actual inserts -- but check for conflict first,
@@ -2361,13 +2368,9 @@ heap_multi_insert(Relation relation, HeapTuple *tuples, int ntuples,
 			CacheInvalidateHeapTuple(relation, heaptuples[i], NULL);
 	}
 
-	/*
-	 * Copy t_self fields back to the caller's original tuples. This does
-	 * nothing for untoasted tuples (tuples[i] == heaptuples[i)], but it's
-	 * probably faster to always copy than check.
-	 */
+	/* copy t_self fields back to the caller's slots */
 	for (i = 0; i < ntuples; i++)
-		tuples[i]->t_self = heaptuples[i]->t_self;
+		slots[i]->tts_tid = heaptuples[i]->t_self;
 
 	pgstat_count_heap_insert(relation, ntuples);
 }
diff --git 

Re: COPY FROM WHEN condition

[Andres Freund]

Hi,

On 2019-01-30 10:33:30 +1300, David Rowley wrote:
> On Wed, 30 Jan 2019 at 10:12, Andres Freund  wrote:
> >
> > On 2019-01-30 10:05:35 +1300, David Rowley wrote:
> > > On Wed, 30 Jan 2019 at 04:22, Andres Freund  wrote:
> > > > I think I might have a patch addressing the problem incidentally. For 
> > > > pluggable storage I slotified copy.c, which also removes the first 
> > > > heap_form_tuple. Quite possible that nothing more is needed. I've 
> > > > removed the batch context altogether in yesterday's rebase, there was 
> > > > no need anymore.
> > >
> > > In your patch, where do the batched tuples get stored before the heap
> > > insert is done?
> >
> > There's one slot for each batched tuple (they are reused). Before
> > materialization the tuples solely exist in tts_isnull/values into which
> > NextCopyFrom() directly parses the values.  Tuples never get extracted
> > from the slot in copy.c itself anymore, table_multi_insert() accepts
> > slots.  Not quite sure whether I've answered your question?
> 
> I think so.  I imagine that should also speed up COPY WHERE too as
> it'll no longer form a tuple before possibly discarding it.

Right.

I found some issues in my patch (stupid implementation of copying from
one slot to the other), but after fixing that I get:

master:
Time: 16013.509 ms (00:16.014)
Time: 16836.110 ms (00:16.836)
Time: 16636.796 ms (00:16.637)

pluggable storage:
Time: 15974.243 ms (00:15.974)
Time: 16183.442 ms (00:16.183)
Time: 16055.192 ms (00:16.055)

(with a truncate between each run)

So that seems a bit better. Albeit at the cost of having a few, on
demand creatd, empty slots for each encountered partition.

I'm pretty sure we can optimize that further...


Greetings,

Andres Freund

Re: COPY FROM WHEN condition

[David Rowley]

On Wed, 30 Jan 2019 at 10:12, Andres Freund  wrote:
>
> On 2019-01-30 10:05:35 +1300, David Rowley wrote:
> > On Wed, 30 Jan 2019 at 04:22, Andres Freund  wrote:
> > > I think I might have a patch addressing the problem incidentally. For 
> > > pluggable storage I slotified copy.c, which also removes the first 
> > > heap_form_tuple. Quite possible that nothing more is needed. I've removed 
> > > the batch context altogether in yesterday's rebase, there was no need 
> > > anymore.
> >
> > In your patch, where do the batched tuples get stored before the heap
> > insert is done?
>
> There's one slot for each batched tuple (they are reused). Before
> materialization the tuples solely exist in tts_isnull/values into which
> NextCopyFrom() directly parses the values.  Tuples never get extracted
> from the slot in copy.c itself anymore, table_multi_insert() accepts
> slots.  Not quite sure whether I've answered your question?

I think so.  I imagine that should also speed up COPY WHERE too as
it'll no longer form a tuple before possibly discarding it.

-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: COPY FROM WHEN condition

[Andres Freund]

Hi,

On 2019-01-30 10:05:35 +1300, David Rowley wrote:
> On Wed, 30 Jan 2019 at 04:22, Andres Freund  wrote:
> > I think I might have a patch addressing the problem incidentally. For 
> > pluggable storage I slotified copy.c, which also removes the first 
> > heap_form_tuple. Quite possible that nothing more is needed. I've removed 
> > the batch context altogether in yesterday's rebase, there was no need 
> > anymore.
> 
> In your patch, where do the batched tuples get stored before the heap
> insert is done?

There's one slot for each batched tuple (they are reused). Before
materialization the tuples solely exist in tts_isnull/values into which
NextCopyFrom() directly parses the values.  Tuples never get extracted
from the slot in copy.c itself anymore, table_multi_insert() accepts
slots.  Not quite sure whether I've answered your question?

Greetings,

Andres Freund

Re: COPY FROM WHEN condition

[David Rowley]

On Tue, 29 Jan 2019 at 22:51, Tomas Vondra  wrote:
>
> On 1/29/19 8:18 AM, David Rowley wrote:
> > So looks like your change slows this code down 4% for this test case.
> > That's about twice as bad as the 2.2% regression that I had to solve
> > for the multi-insert partition patch (of which you've removed much of
> > the code from)
> >
> > I'd really like to see the alternating batch context code being put
> > back in to fix this. Of course, if you have a better idea, then we can
> > look into that, but just removing code that was carefully written and
> > benchmarked without any new benchmarks to prove that it's okay to do
> > so seems wrong to me.
> >
>
> Yeah, that's not very nice. Do you suggest to revert 36a1281f86c0f, or
> shall we try to massage it a bit first? ISTM we could simply create two
> batch memory contexts and alternate those, just like with the expression
> contexts before.

I don't think a revert should be done. I didn't check in detail, but
it seems what you did fixed the memory leak issue for when many tuples
are filtered out.  I'd like to have more detail on what Andres is
proposing, but otherwise, I'd imagine we can just have two batch
contexts and alternate between them, as before, but also keep the
per-tuple context too.

-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: COPY FROM WHEN condition

[David Rowley]

On Wed, 30 Jan 2019 at 04:22, Andres Freund  wrote:
> I think I might have a patch addressing the problem incidentally. For 
> pluggable storage I slotified copy.c, which also removes the first 
> heap_form_tuple. Quite possible that nothing more is needed. I've removed the 
> batch context altogether in yesterday's rebase, there was no need anymore.

In your patch, where do the batched tuples get stored before the heap
insert is done?

-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: COPY FROM WHEN condition

[David Rowley]

On Wed, 30 Jan 2019 at 03:26, Tomas Vondra  wrote:
>
> On 1/29/19 8:18 AM, David Rowley wrote:
> > (details about performance regression)
>
> How do I reproduce this? I don't see this test in the thread you linked,
> so I'm not sure how many partitions you were using, what's the structure
> of the table etc.

The setup is:

create table listp (a int) partition by list(a);
create table listp1 partition of listp for values in(1);
create table listp2 partition of listp for values in(2);


-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: COPY FROM WHEN condition

[Andres Freund]

Hi,

On January 29, 2019 6:25:59 AM PST, Tomas Vondra  
wrote:
>On 1/29/19 8:18 AM, David Rowley wrote:
>> ...
>> Here are my performance tests of with and without your change to the
>> memory contexts (I missed where you posted your results).
>> 
>> $ cat bench.pl
>> for (my $i=0; $i < 8912891; $i++) {
>> print "1\n1\n2\n2\n";
>> }
>> 36a1281f86c: (with your change)
>> 
>> postgres=# copy listp from program $$perl ~/bench.pl$$ delimiter '|';
>> COPY 35651564
>> Time: 26825.142 ms (00:26.825)
>> Time: 27202.117 ms (00:27.202)
>> Time: 26266.705 ms (00:26.267)
>> 
>> 4be058fe9ec: (before your change)
>> 
>> postgres=# copy listp from program $$perl ~/bench.pl$$ delimiter '|';
>> COPY 35651564
>> Time: 25645.460 ms (00:25.645)
>> Time: 25698.193 ms (00:25.698)
>> Time: 25737.251 ms (00:25.737)
>> 
>
>How do I reproduce this? I don't see this test in the thread you
>linked,
>so I'm not sure how many partitions you were using, what's the
>structure
>of the table etc.

I think I might have a patch addressing the problem incidentally. For pluggable 
storage I slotified copy.c, which also removes the first heap_form_tuple. Quite 
possible that nothing more is needed. I've removed the batch context altogether 
in yesterday's rebase, there was no need anymore.

Andres
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: COPY FROM WHEN condition

[Tomas Vondra]

On 1/29/19 8:18 AM, David Rowley wrote:
> ...
> Here are my performance tests of with and without your change to the
> memory contexts (I missed where you posted your results).
> 
> $ cat bench.pl
> for (my $i=0; $i < 8912891; $i++) {
> print "1\n1\n2\n2\n";
> }
> 36a1281f86c: (with your change)
> 
> postgres=# copy listp from program $$perl ~/bench.pl$$ delimiter '|';
> COPY 35651564
> Time: 26825.142 ms (00:26.825)
> Time: 27202.117 ms (00:27.202)
> Time: 26266.705 ms (00:26.267)
> 
> 4be058fe9ec: (before your change)
> 
> postgres=# copy listp from program $$perl ~/bench.pl$$ delimiter '|';
> COPY 35651564
> Time: 25645.460 ms (00:25.645)
> Time: 25698.193 ms (00:25.698)
> Time: 25737.251 ms (00:25.737)
> 

How do I reproduce this? I don't see this test in the thread you linked,
so I'm not sure how many partitions you were using, what's the structure
of the table etc.

regards

-- 
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: COPY FROM WHEN condition

[Tomas Vondra]

On 1/29/19 8:18 AM, David Rowley wrote:
> On Wed, 23 Jan 2019 at 06:35, Tomas Vondra  
> wrote:
>> It turned out to be a tad more complex due to partitioning, because when
>> we find the partitions do not match, the tuple is already allocated in
>> the "current" context (be it per-tuple or batch). So we can't just free
>> the whole context at that point. The old code worked around this by
>> alternating two contexts, but that seems a bit too cumbersome to me, so
>> the patch simply copies the tuple to the new context. That allows us to
>> reset the batch context always, right after emptying the buffer. I need
>> to do some benchmarking to see if the extra copy causes any regression.
> 
> I agree that fixing the problem mentioned by separating out tuple and
> batch contexts is a good idea, but I disagree with removing the
> alternating batch context idea.  FWIW the reason the alternating
> contexts went in wasn't just for fun, it was on performance grounds.
> There's a lengthy discussion in [1] about not adding any new
> performance regressions to COPY. To be more specific, Peter complains
> about a regression of 5% in [2].
> 
> It's pretty disheartening to see the work there being partially undone.
> 

Whoops :-(

> Here are my performance tests of with and without your change to the
> memory contexts (I missed where you posted your results).
> 
> $ cat bench.pl
> for (my $i=0; $i < 8912891; $i++) {
> print "1\n1\n2\n2\n";
> }
> 36a1281f86c: (with your change)
> 
> postgres=# copy listp from program $$perl ~/bench.pl$$ delimiter '|';
> COPY 35651564
> Time: 26825.142 ms (00:26.825)
> Time: 27202.117 ms (00:27.202)
> Time: 26266.705 ms (00:26.267)
> 
> 4be058fe9ec: (before your change)
> 
> postgres=# copy listp from program $$perl ~/bench.pl$$ delimiter '|';
> COPY 35651564
> Time: 25645.460 ms (00:25.645)
> Time: 25698.193 ms (00:25.698)
> Time: 25737.251 ms (00:25.737)
> 
> So looks like your change slows this code down 4% for this test case.
> That's about twice as bad as the 2.2% regression that I had to solve
> for the multi-insert partition patch (of which you've removed much of
> the code from)
> 
> I'd really like to see the alternating batch context code being put
> back in to fix this. Of course, if you have a better idea, then we can
> look into that, but just removing code that was carefully written and
> benchmarked without any new benchmarks to prove that it's okay to do
> so seems wrong to me.
> 

Yeah, that's not very nice. Do you suggest to revert 36a1281f86c0f, or
shall we try to massage it a bit first? ISTM we could simply create two
batch memory contexts and alternate those, just like with the expression
contexts before.


regards

-- 
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: COPY FROM WHEN condition

[David Rowley]

On Wed, 23 Jan 2019 at 06:35, Tomas Vondra  wrote:
> It turned out to be a tad more complex due to partitioning, because when
> we find the partitions do not match, the tuple is already allocated in
> the "current" context (be it per-tuple or batch). So we can't just free
> the whole context at that point. The old code worked around this by
> alternating two contexts, but that seems a bit too cumbersome to me, so
> the patch simply copies the tuple to the new context. That allows us to
> reset the batch context always, right after emptying the buffer. I need
> to do some benchmarking to see if the extra copy causes any regression.

I agree that fixing the problem mentioned by separating out tuple and
batch contexts is a good idea, but I disagree with removing the
alternating batch context idea.  FWIW the reason the alternating
contexts went in wasn't just for fun, it was on performance grounds.
There's a lengthy discussion in [1] about not adding any new
performance regressions to COPY. To be more specific, Peter complains
about a regression of 5% in [2].

It's pretty disheartening to see the work there being partially undone.

Here are my performance tests of with and without your change to the
memory contexts (I missed where you posted your results).

$ cat bench.pl
for (my $i=0; $i < 8912891; $i++) {
print "1\n1\n2\n2\n";
}
36a1281f86c: (with your change)

postgres=# copy listp from program $$perl ~/bench.pl$$ delimiter '|';
COPY 35651564
Time: 26825.142 ms (00:26.825)
Time: 27202.117 ms (00:27.202)
Time: 26266.705 ms (00:26.267)

4be058fe9ec: (before your change)

postgres=# copy listp from program $$perl ~/bench.pl$$ delimiter '|';
COPY 35651564
Time: 25645.460 ms (00:25.645)
Time: 25698.193 ms (00:25.698)
Time: 25737.251 ms (00:25.737)

So looks like your change slows this code down 4% for this test case.
That's about twice as bad as the 2.2% regression that I had to solve
for the multi-insert partition patch (of which you've removed much of
the code from)

I'd really like to see the alternating batch context code being put
back in to fix this. Of course, if you have a better idea, then we can
look into that, but just removing code that was carefully written and
benchmarked without any new benchmarks to prove that it's okay to do
so seems wrong to me.

[1] 
https://www.postgresql.org/message-id/flat/CAKJS1f93DeHN%2B9RrD9jYn0iF_o89w2B%2BU8-Ao5V1kd8Cf7oSGQ%40mail.gmail.com
[2] 
https://www.postgresql.org/message-id/2b40468d-6be0-e795-c363-0015bc9fa747%402ndquadrant.com

-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: COPY FROM WHEN condition

[Tomas Vondra]

On 1/22/19 10:00 AM, Surafel Temesgen wrote:
> 
> 
> On Mon, Jan 21, 2019 at 6:22 PM Tomas Vondra
> mailto:tomas.von...@2ndquadrant.com>> wrote:
> 
> 
> I think the condition can be just
> 
>     if (contain_volatile_functions(cstate->whereClause)) { ... }
> 
> 

I've pushed a fix for the volatility check.

Attached is a patch for the other issue, creating a separate batch
context long the lines outlined in the previous email. It's a bit too
late for me to push it now, especially right before a couple of days
off. So I'll push that in a couple of days.


regards

-- 
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 03745cca75..41dbcd5b42 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -2323,9 +2323,9 @@ CopyFrom(CopyState cstate)
 	ExprContext *econtext;
 	TupleTableSlot *myslot;
 	MemoryContext oldcontext = CurrentMemoryContext;
+	MemoryContext batchcontext;
 
 	PartitionTupleRouting *proute = NULL;
-	ExprContext *secondaryExprContext = NULL;
 	ErrorContextCallback errcallback;
 	CommandId	mycid = GetCurrentCommandId(true);
 	int			hi_options = 0; /* start with default heap_insert options */
@@ -2639,20 +2639,10 @@ CopyFrom(CopyState cstate)
 		 * Normally, when performing bulk inserts we just flush the insert
 		 * buffer whenever it becomes full, but for the partitioned table
 		 * case, we flush it whenever the current tuple does not belong to the
-		 * same partition as the previous tuple, and since we flush the
-		 * previous partition's buffer once the new tuple has already been
-		 * built, we're unable to reset the estate since we'd free the memory
-		 * in which the new tuple is stored.  To work around this we maintain
-		 * a secondary expression context and alternate between these when the
-		 * partition changes.  This does mean we do store the first new tuple
-		 * in a different context than subsequent tuples, but that does not
-		 * matter, providing we don't free anything while it's still needed.
+		 * same partition as the previous tuple.
 		 */
 		if (proute)
-		{
 			insertMethod = CIM_MULTI_CONDITIONAL;
-			secondaryExprContext = CreateExprContext(estate);
-		}
 		else
 			insertMethod = CIM_MULTI;
 
@@ -2685,6 +2675,14 @@ CopyFrom(CopyState cstate)
 	errcallback.previous = error_context_stack;
 	error_context_stack = 
 
+	/*
+	 * Set up memory context for batches. For cases without batching we could
+	 * use the per-tuple context, but it does not seem worth the complexity.
+	 */
+	batchcontext = AllocSetContextCreate(CurrentMemoryContext,
+		 "batch context",
+		 ALLOCSET_DEFAULT_SIZES);
+
 	for (;;)
 	{
 		TupleTableSlot *slot;
@@ -2692,18 +2690,14 @@ CopyFrom(CopyState cstate)
 
 		CHECK_FOR_INTERRUPTS();
 
-		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);
-		}
+		/*
+		 * Reset the per-tuple exprcontext. We do this after every tuple, to
+		 * clean-up after expression evaluations etc.
+		 */
+		ResetPerTupleExprContext(estate);
 
-		/* Switch into its memory context */
-		MemoryContextSwitchTo(GetPerTupleMemoryContext(estate));
+		/* Switch into per-batch memory context. */
+		MemoryContextSwitchTo(batchcontext);
 
 		if (!NextCopyFrom(cstate, econtext, values, nulls))
 			break;
@@ -2756,7 +2750,7 @@ CopyFrom(CopyState cstate)
 	 */
 	if (nBufferedTuples > 0)
 	{
-		ExprContext *swapcontext;
+		MemoryContext	oldcontext;
 
 		CopyFromInsertBatch(cstate, estate, mycid, hi_options,
 			prevResultRelInfo, myslot, bistate,
@@ -2765,29 +2759,26 @@ CopyFrom(CopyState cstate)
 		nBufferedTuples = 0;
 		bufferedTuplesSize = 0;
 
-		Assert(secondaryExprContext);
-
 		/*
-		 * Normally we reset the per-tuple context whenever
-		 * the bufferedTuples array is empty at the beginning
-		 * of the loop, however, it is possible since we flush
-		 * the buffer here that the buffer is never empty at
-		 * the start of the loop.  To prevent the per-tuple
-		 * context from never being reset we maintain a second
-		 * context and alternate between them when the
-		 * partition changes.  We can now reset
-		 * secondaryExprContext as this is no longer needed,
-		 * since we just flushed any tuples stored in it.  We
-		 * also now switch over to the other context.  This
-		 * does mean that the first tuple in the buffer won't
-		 * be in the same context as the others, but that does
-		 * not matter since we only reset it after the flush.
+		 * The tuple is allocated in the batch context, which we
+		 * want to reset. So to keep the tuple we copy the tuple
+		 * 

Re: COPY FROM WHEN condition

[Andres Freund]

Hi,

On 2019-01-22 18:35:21 +0100, Tomas Vondra wrote:
> On 1/21/19 11:15 PM, Tomas Vondra wrote:
> > On 1/21/19 7:51 PM, Andres Freund wrote:
> >> I'm *not* convinced by this. I think it's bad enough that we do this for
> >> normal COPY, but for WHEN, we could end up *never* resetting before the
> >> end. Consider a case where a single tuple is inserted, and then *all*
> >> rows are filtered.  I think this needs a separate econtext that's reset
> >> every round. Or alternatively you could fix the code not to rely on
> >> per-tuple not being reset when tuples are buffered - that actually ought
> >> to be fairly simple.
> >>
> > 
> > I think separating the per-tuple and per-batch contexts is the right
> > thing to do, here. It seems the batching was added somewhat later and
> > using the per-tuple context is rather confusing.
> > 
> 
> OK, here is a WIP patch doing that. It creates a new "batch" context,
> and allocates tuples in it (instead of the per-tuple context). The
> per-tuple context is now reset always, irrespectedly of nBufferedTuples.
> And the batch context is reset every time the batch is emptied.
> 
> It turned out to be a tad more complex due to partitioning, because when
> we find the partitions do not match, the tuple is already allocated in
> the "current" context (be it per-tuple or batch). So we can't just free
> the whole context at that point. The old code worked around this by
> alternating two contexts, but that seems a bit too cumbersome to me, so
> the patch simply copies the tuple to the new context. That allows us to
> reset the batch context always, right after emptying the buffer. I need
> to do some benchmarking to see if the extra copy causes any regression.
> 
> Overall, separating the contexts makes it quite a bit clearer. I'm not
> entirely happy about the per-tuple context being "implicit" (hidden in
> executor context) while the batch context being explicitly created, but
> there's not much I can do about that.

I think the extra copy is probably OK for now - as part of the pluggable
storage series I've converted COPY to use slots, which should make that
less of an issue - I've not done that, but I actually assume we could
remove the whole batch context afterwards.

Greetings,

Andres Freund

Re: COPY FROM WHEN condition

[Tomas Vondra]

On 1/21/19 11:15 PM, Tomas Vondra wrote:
> 
> 
> On 1/21/19 7:51 PM, Andres Freund wrote:
>> Hi,
>>
>> On 2019-01-21 16:22:11 +0100, Tomas Vondra wrote:
>>>
>>>
>>> On 1/21/19 4:33 AM, Tomas Vondra wrote:


 On 1/21/19 3:12 AM, Andres Freund wrote:
> 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
>>> >> > 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, );
>
>
> 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.

 Actually, no. The ResetPerTupleExprContext boils down to

 MemoryContextReset((econtext)->ecxt_per_tuple_memory)

 and ExecEvalExprSwitchContext does this

 MemoryContextSwitchTo(econtext->ecxt_per_tuple_memory);

 So it's resetting the right context, although only on batch boundary.
>>
> Seems just using ExecQualAndReset() ought to be sufficient?
>

 That may still be the right thing to do.

>>>
>>> Actually, no, because that would reset the context far too early (and
>>> it's easy to trigger segfaults). So the reset would have to happen after
>>> processing the row, not this early.
>>
>> Yea, sorry, I was too tired yesterday evening. I'd spent 10h splitting
>> up the pluggable storage patch into individual pieces...
>>
>>
>>> But I think the current behavior is actually OK, as it matches what we
>>> do for defexprs. And the comment before ResetPerTupleExprContext says this:
>>>
>>> /*
>>>  * 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.)
>>>  */
>>>
>>> So the per-tuple context is not quite per-tuple anyway. Sure, we might
>>> rework that but I don't 

Re: COPY FROM WHEN condition

[Surafel Temesgen]

On Mon, Jan 21, 2019 at 6:22 PM Tomas Vondra 
wrote:

>
> I think the condition can be just
>
> if (contain_volatile_functions(cstate->whereClause)) { ... }
>
>
>
yes it can be.

regards
Surafel

Re: COPY FROM WHEN condition

[Tomas Vondra]

On 1/21/19 7:51 PM, Andres Freund wrote:
> Hi,
> 
> On 2019-01-21 16:22:11 +0100, Tomas Vondra wrote:
>>
>>
>> On 1/21/19 4:33 AM, Tomas Vondra wrote:
>>>
>>>
>>> On 1/21/19 3:12 AM, Andres Freund wrote:
 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
>> mailto:tomas.von...@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, );


 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.
>>>
>>> Actually, no. The ResetPerTupleExprContext boils down to
>>>
>>> MemoryContextReset((econtext)->ecxt_per_tuple_memory)
>>>
>>> and ExecEvalExprSwitchContext does this
>>>
>>> MemoryContextSwitchTo(econtext->ecxt_per_tuple_memory);
>>>
>>> So it's resetting the right context, although only on batch boundary.
> 
 Seems just using ExecQualAndReset() ought to be sufficient?

>>>
>>> That may still be the right thing to do.
>>>
>>
>> Actually, no, because that would reset the context far too early (and
>> it's easy to trigger segfaults). So the reset would have to happen after
>> processing the row, not this early.
> 
> Yea, sorry, I was too tired yesterday evening. I'd spent 10h splitting
> up the pluggable storage patch into individual pieces...
> 
> 
>> But I think the current behavior is actually OK, as it matches what we
>> do for defexprs. And the comment before ResetPerTupleExprContext says this:
>>
>> /*
>>  * 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.)
>>  */
>>
>> So the per-tuple context is not quite per-tuple anyway. Sure, we might
>> rework that but I don't think that's an issue in this patch.
> 
> I'm *not* convinced by this. I think it's bad enough that we do this for
> normal COPY, but for WHEN, we could end up 

Re: COPY FROM WHEN condition

[Andres Freund]

Hi,

On 2019-01-21 16:22:11 +0100, Tomas Vondra wrote:
> 
> 
> On 1/21/19 4:33 AM, Tomas Vondra wrote:
> > 
> > 
> > On 1/21/19 3:12 AM, Andres Freund wrote:
> >> 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
>  mailto:tomas.von...@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, );
> >>
> >>
> >> 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.
> > 
> > Actually, no. The ResetPerTupleExprContext boils down to
> > 
> > MemoryContextReset((econtext)->ecxt_per_tuple_memory)
> > 
> > and ExecEvalExprSwitchContext does this
> > 
> > MemoryContextSwitchTo(econtext->ecxt_per_tuple_memory);
> > 
> > So it's resetting the right context, although only on batch boundary.

> >> Seems just using ExecQualAndReset() ought to be sufficient?
> >>
> > 
> > That may still be the right thing to do.
> > 
> 
> Actually, no, because that would reset the context far too early (and
> it's easy to trigger segfaults). So the reset would have to happen after
> processing the row, not this early.

Yea, sorry, I was too tired yesterday evening. I'd spent 10h splitting
up the pluggable storage patch into individual pieces...


> But I think the current behavior is actually OK, as it matches what we
> do for defexprs. And the comment before ResetPerTupleExprContext says this:
> 
> /*
>  * 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.)
>  */
> 
> So the per-tuple context is not quite per-tuple anyway. Sure, we might
> rework that but I don't think that's an issue in this patch.

I'm *not* convinced by this. I think it's bad enough that we do this for
normal COPY, but for WHEN, we could end up *never* resetting before the
end. Consider a case where a single tuple is 

Re: COPY FROM WHEN condition

[Tomas Vondra]

On 1/21/19 4:33 AM, Tomas Vondra wrote:
> 
> 
> On 1/21/19 3:12 AM, Andres Freund wrote:
>> 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
 mailto:tomas.von...@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, );
>>
>>
>> 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.
> 
> Actually, no. The ResetPerTupleExprContext boils down to
> 
> MemoryContextReset((econtext)->ecxt_per_tuple_memory)
> 
> and ExecEvalExprSwitchContext does this
> 
> MemoryContextSwitchTo(econtext->ecxt_per_tuple_memory);
> 
> So it's resetting the right context, although only on batch boundary.
> But now I see 31f38174 does this:
> 
> else if (cstate->whereClause != NULL ||
>  contain_volatile_functions(cstate->whereClause))
> {
> ...
> insertMethod = CIM_SINGLE;
> }
> 
> so it does not do batching with WHERE. But the condition seems wrong, I
> guess it should be && instead of ||. Will investigate in the morning.
> 

I think the condition can be just

if (contain_volatile_functions(cstate->whereClause)) { ... }

Per the attached patch. Surafel, do you agree?


>> Seems just using ExecQualAndReset() ought to be sufficient?
>>
> 
> That may still be the right thing to do.
> 

Actually, no, because that would reset the context far too early (and
it's easy to trigger segfaults). So the reset would have to happen after
processing the row, not this early.

But I think the current behavior is actually OK, as it matches what we
do for defexprs. And the comment before ResetPerTupleExprContext says this:

/*
 * 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.)
 */

So the per-tuple context is not quite per-tuple anyway. Sure, we might
rework that but I don't think that's an issue in this patch.

regards

-- 
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL 

Re: COPY FROM WHEN condition

[Tomas Vondra]

On 1/21/19 3:12 AM, Andres Freund wrote:
> 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
>>> mailto:tomas.von...@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, );
> 
> 
> 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.

Actually, no. The ResetPerTupleExprContext boils down to

MemoryContextReset((econtext)->ecxt_per_tuple_memory)

and ExecEvalExprSwitchContext does this

MemoryContextSwitchTo(econtext->ecxt_per_tuple_memory);

So it's resetting the right context, although only on batch boundary.
But now I see 31f38174 does this:

else if (cstate->whereClause != NULL ||
 contain_volatile_functions(cstate->whereClause))
{
...
insertMethod = CIM_SINGLE;
}

so it does not do batching with WHERE. But the condition seems wrong, I
guess it should be && instead of ||. Will investigate in the morning.

> Seems just using ExecQualAndReset() ought to be sufficient?
> 

That may still be the right thing to do.


cheers

-- 
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: COPY FROM WHEN condition

[Tomas Vondra]

On 1/21/19 3:12 AM, Andres Freund wrote:
> 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
>>> mailto:tomas.von...@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, );
> 
> 
> 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.

So context, much simple. Wow.
 > Seems just using ExecQualAndReset() ought to be sufficient?
> 

Seems like it.


regards

-- 
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: COPY FROM WHEN condition

[Andres Freund]

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
> > > > > >  > > > > > > 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, );


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

Re: COPY FROM WHEN condition

[Andres Freund]

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
> > > > > mailto:tomas.von...@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.

Greetings,

Andres Freund

Re: COPY FROM WHEN condition

[Tomas Vondra]

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
mailto:tomas.von...@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.


cheers

--
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: COPY FROM WHEN condition

[Andres Freund]

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
> >> mailto:tomas.von...@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?

Greetings,

Andres Freund

Re: COPY FROM WHEN condition

[Surafel Temesgen]

On Sun, Jan 20, 2019 at 2:24 AM Tomas Vondra 
wrote:

>
> Pushed, thanks for the patch.
>
> cheers
>
>
>
Thank you

regards
Surafel

Re: COPY FROM WHEN condition

[Tomas Vondra]

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
>> mailto:tomas.von...@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.

cheers

-- 
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: COPY FROM WHEN condition

[Tomas Vondra]

On 12/13/18 8:09 AM, Surafel Temesgen wrote:
> 
> 
> On Wed, Dec 12, 2018 at 9:28 PM Tomas Vondra
> mailto:tomas.von...@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.

regards

-- 
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: COPY FROM WHEN condition

[Surafel Temesgen]

On Wed, Dec 12, 2018 at 9:28 PM Tomas Vondra 
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*

*regards *

*Surafel*
diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
index 411941ed31..4e6eb1fcdb 100644
--- a/doc/src/sgml/ref/copy.sgml
+++ b/doc/src/sgml/ref/copy.sgml
@@ -25,6 +25,7 @@ PostgreSQL documentation
 COPY table_name [ ( column_name [, ...] ) ]
 FROM { 'filename' | PROGRAM 'command' | STDIN }
 [ [ WITH ] ( option [, ...] ) ]
+[ WHERE condition ]
 
 COPY { table_name [ ( column_name [, ...] ) ] | ( query ) }
 TO { 'filename' | PROGRAM 'command' | STDOUT }
@@ -353,6 +354,31 @@ COPY { table_name [ ( 

 
+   
+WHERE
+
+   
+The optional WHERE clause has the general form
+
+WHERE condition
+
+where condition is
+any expression that evaluates to a result of type
+boolean.  Any row that does not satisfy this
+condition will not be inserted to the table.  A row satisfies the
+condition if it returns true when the actual row values are
+substituted for any variable references.
+   
+
+   
+Currently, WHERE expressions cannot contain
+subqueries and will not see changes made by COPY
+command in a case of VOLATILE function.
+   
+
+
+   
+
   
  
 
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 4aa8890fe8..b0471eedab 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -40,7 +40,11 @@
 #include "miscadmin.h"
 #include "optimizer/clauses.h"
 #include "optimizer/planner.h"
+#include "optimizer/prep.h"
 #include "nodes/makefuncs.h"
+#include "parser/parse_coerce.h"
+#include "parser/parse_collate.h"
+#include "parser/parse_expr.h"
 #include "parser/parse_relation.h"
 #include "port/pg_bswap.h"
 #include "rewrite/rewriteHandler.h"
@@ -150,6 +154,7 @@ typedef struct CopyStateData
 	bool		convert_selectively;	/* do selective binary conversion? */
 	List	   *convert_select; /* list of column names (can be NIL) */
 	bool	   *convert_select_flags;	/* per-column CSV/TEXT CS flags */
+	Node	   *whereClause;	/* WHERE condition (or NULL) */
 
 	/* these are just for error messages, see CopyFromErrorCallback */
 	const char *cur_relname;	/* table name for error messages */
@@ -180,6 +185,7 @@ typedef struct CopyStateData
 	ExprState **defexprs;		/* array of default att expressions */
 	bool		volatile_defexprs;	/* is any of defexprs volatile? */
 	List	   *range_table;
+	ExprState  *qualexpr;
 
 	TransitionCaptureState *transition_capture;
 
@@ -801,6 +807,7 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt,
 	Relation	rel;
 	Oid			relid;
 	RawStmt*query = NULL;
+	Node*whereClause = NULL;
 
 	/*
 	 * Disallow COPY to/from file or program except to users with the
@@ -854,6 +861,25 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt,
 			NULL, false, false);
 		rte->requiredPerms = (is_from ? ACL_INSERT : ACL_SELECT);
 
+		if (stmt->whereClause)
+		{
+			/* add rte to column namespace  */
+			addRTEtoQuery(pstate, rte, false, true, true);
+
+			/* Transform the raw expression tree */
+			whereClause = transformExpr(pstate, stmt->whereClause, EXPR_KIND_COPY_WHERE);
+
+			/*  Make sure it yields a boolean result. */
+			whereClause = coerce_to_boolean(pstate, whereClause, "WHERE");
+			/* we have to fix its collations too */
+			assign_expr_collations(pstate, whereClause);
+
+			whereClause = eval_const_expressions(NULL, whereClause);
+
+			whereClause = (Node *) canonicalize_qual((Expr *) whereClause, false);
+			whereClause = (Node *) make_ands_implicit((Expr *) whereClause);
+		}
+
 		tupDesc = RelationGetDescr(rel);
 		attnums = CopyGetAttnums(tupDesc, rel, stmt->attlist);
 		foreach(cur, attnums)
@@ -1002,6 +1028,7 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt,
 
 		cstate = BeginCopyFrom(pstate, rel, stmt->filename, stmt->is_program,
 			   NULL, stmt->attlist, stmt->options);
+		cstate->whereClause = whereClause;
 		*processed = CopyFrom(cstate);	/* copy from file to database */
 		EndCopyFrom(cstate);
 	}
@@ -2531,6 +2558,10 @@ CopyFrom(CopyState cstate)
 	if (cstate->rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
 		proute = ExecSetupPartitionTupleRouting(NULL, cstate->rel);
 
+	if (cstate->whereClause)
+		cstate->qualexpr = ExecInitQual(castNode(List, cstate->whereClause),
+>ps);
+
 	/*
 	 * It's more efficient to prepare a bunch of tuples for insertion, and
 	 * insert them in one heap_multi_insert() call, than call heap_insert()
@@ -2576,6 +2607,16 @@ CopyFrom(CopyState cstate)
 		 */
 		insertMethod = CIM_SINGLE;
 	}
+	else if (cstate->whereClause != NULL ||
+			 contain_volatile_functions(cstate->whereClause))
+	{
+		/*
+		 * Can't support multi-inserts if there are any volatile funcation
+		 * expressions in WHERE clause.  Similarly to the trigger case above,
+		

Re: COPY FROM WHEN condition

[Tomas Vondra]

On 12/12/18 7:05 AM, Surafel Temesgen wrote:
> 
> 
> On Tue, Dec 4, 2018 at 12:44 PM Alvaro Herrera  > wrote:
> 
> After reading this thread, I think I like WHERE better than FILTER.
> Tally:
> 
> WHERE: Adam Berlin, Lim Myungkyu, Dean Rasheed, yours truly
> FILTER: Tomas Vondra, Surafel Temesgen
> 
> 
> 
> accepting tally result  i change the keyword to WHERE condition and
> allow volatile function with single insertion method

Can you also update the docs to mention that the functions called from
the WHERE clause does not see effects of the COPY itself?

regards

-- 
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


pEpkey.asc
Description: application/pgp-keys

Re: COPY FROM WHEN condition

[Surafel Temesgen]

On Tue, Dec 4, 2018 at 12:44 PM Alvaro Herrera 
wrote:

> After reading this thread, I think I like WHERE better than FILTER.
> Tally:
>
> WHERE: Adam Berlin, Lim Myungkyu, Dean Rasheed, yours truly
> FILTER: Tomas Vondra, Surafel Temesgen
>
>
>
accepting tally result  i change the keyword to WHERE condition and allow
volatile function
with single insertion method
regards
Surafel
diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
index 411941ed31..c1a7874fb7 100644
--- a/doc/src/sgml/ref/copy.sgml
+++ b/doc/src/sgml/ref/copy.sgml
@@ -25,6 +25,7 @@ PostgreSQL documentation
 COPY table_name [ ( column_name [, ...] ) ]
 FROM { 'filename' | PROGRAM 'command' | STDIN }
 [ [ WITH ] ( option [, ...] ) ]
+[ WHERE condition ]
 
 COPY { table_name [ ( column_name [, ...] ) ] | ( query ) }
 TO { 'filename' | PROGRAM 'command' | STDOUT }
@@ -353,6 +354,30 @@ COPY { table_name [ ( 

 
+   
+WHERE
+
+   
+The optional WHERE clause has the general form
+
+WHERE condition
+
+where condition is
+any expression that evaluates to a result of type
+boolean.  Any row that does not satisfy this
+condition will not be inserted to the table.  A row satisfies the
+condition if it returns true when the actual row values are
+substituted for any variable references.
+   
+
+   
+Currently, WHERE expressions cannot contain
+subqueries.
+   
+
+
+   
+
   
  
 
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 4aa8890fe8..7150bfc43a 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -40,7 +40,11 @@
 #include "miscadmin.h"
 #include "optimizer/clauses.h"
 #include "optimizer/planner.h"
+#include "optimizer/prep.h"
 #include "nodes/makefuncs.h"
+#include "parser/parse_coerce.h"
+#include "parser/parse_collate.h"
+#include "parser/parse_expr.h"
 #include "parser/parse_relation.h"
 #include "port/pg_bswap.h"
 #include "rewrite/rewriteHandler.h"
@@ -150,6 +154,7 @@ typedef struct CopyStateData
 	bool		convert_selectively;	/* do selective binary conversion? */
 	List	   *convert_select; /* list of column names (can be NIL) */
 	bool	   *convert_select_flags;	/* per-column CSV/TEXT CS flags */
+	Node	   *whereClause;	/* WHERE condition (or NULL) */
 
 	/* these are just for error messages, see CopyFromErrorCallback */
 	const char *cur_relname;	/* table name for error messages */
@@ -180,6 +185,7 @@ typedef struct CopyStateData
 	ExprState **defexprs;		/* array of default att expressions */
 	bool		volatile_defexprs;	/* is any of defexprs volatile? */
 	List	   *range_table;
+	ExprState  *qualexpr;
 
 	TransitionCaptureState *transition_capture;
 
@@ -801,6 +807,7 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt,
 	Relation	rel;
 	Oid			relid;
 	RawStmt*query = NULL;
+	Node*whereClause = NULL;
 
 	/*
 	 * Disallow COPY to/from file or program except to users with the
@@ -854,6 +861,25 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt,
 			NULL, false, false);
 		rte->requiredPerms = (is_from ? ACL_INSERT : ACL_SELECT);
 
+		if (stmt->whereClause)
+		{
+			/* add rte to column namespace  */
+			addRTEtoQuery(pstate, rte, false, true, true);
+
+			/* Transform the raw expression tree */
+			whereClause = transformExpr(pstate, stmt->whereClause, EXPR_KIND_COPY_WHERE);
+
+			/*  Make sure it yields a boolean result. */
+			whereClause = coerce_to_boolean(pstate, whereClause, "WHERE");
+			/* we have to fix its collations too */
+			assign_expr_collations(pstate, whereClause);
+
+			whereClause = eval_const_expressions(NULL, whereClause);
+
+			whereClause = (Node *) canonicalize_qual((Expr *) whereClause, false);
+			whereClause = (Node *) make_ands_implicit((Expr *) whereClause);
+		}
+
 		tupDesc = RelationGetDescr(rel);
 		attnums = CopyGetAttnums(tupDesc, rel, stmt->attlist);
 		foreach(cur, attnums)
@@ -1002,6 +1028,7 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt,
 
 		cstate = BeginCopyFrom(pstate, rel, stmt->filename, stmt->is_program,
 			   NULL, stmt->attlist, stmt->options);
+		cstate->whereClause = whereClause;
 		*processed = CopyFrom(cstate);	/* copy from file to database */
 		EndCopyFrom(cstate);
 	}
@@ -2531,6 +2558,10 @@ CopyFrom(CopyState cstate)
 	if (cstate->rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
 		proute = ExecSetupPartitionTupleRouting(NULL, cstate->rel);
 
+	if (cstate->whereClause)
+		cstate->qualexpr = ExecInitQual(castNode(List, cstate->whereClause),
+>ps);
+
 	/*
 	 * It's more efficient to prepare a bunch of tuples for insertion, and
 	 * insert them in one heap_multi_insert() call, than call heap_insert()
@@ -2576,6 +2607,11 @@ CopyFrom(CopyState cstate)
 		 */
 		insertMethod = CIM_SINGLE;
 	}
+	else if (cstate->whereClause != NULL ||
+			 contain_volatile_functions(cstate->whereClause))
+	{
+		insertMethod = CIM_SINGLE;
+	}
 	else
 	{
 		/*
@@ -2679,6 +2715,13 @@ CopyFrom(CopyState cstate)
 		slot = myslot;
 		

Re: COPY FROM WHEN condition

[Tomas Vondra]

On 12/6/18 4:52 PM, Robert Haas wrote:
> On Wed, Nov 28, 2018 at 6:17 PM Tomas Vondra
>  wrote:
>>> Comparing with overhead of setting snapshot before evaluating every row
>>> and considering this
>>>
>>> kind of usage is not frequent it seems to me the behavior is acceptable
>>
>> I'm not really buying the argument that this behavior is acceptable
>> simply because using the feature like this will be uncommon. That seems
>> like a rather weak reason to accept it.
>>
>> I however agree we don't want to make COPY less efficient, at least not
>> unless absolutely necessary. But I think we can handle this simply by
>> restricting what's allowed to appear the FILTER clause.
>>
>> It should be fine to allow IMMUTABLE and STABLE functions, but not
>> VOLATILE ones. That should fix the example I shared, because f() would
>> not be allowed.
> 
> I don't think that's a very good solution.  It's perfectly sensible
> for someone to want to do WHERE/FILTER random() < 0.01 to load a
> smattering of rows, and this would rule that out for no very good
> reason.
> 

Good point. I agree that's a much more plausible use case for this
feature, and forbidding volatile functions would break it.

> I think it would be fine to just document that if the filter condition
> examines the state of the database, it will not see the results of the
> COPY which is in progress.  I'm pretty sure there are other cases -
> for example with triggers - where you can get code to run that can't
> see the results of the command currently in progress, so I don't
> really buy the idea that having a feature which works that way is
> categorically unacceptable.
> 
> I agree that we can't justify flagrantly wrong behavior on the grounds
> that correct behavior is expensive or on the grounds that the
> incorrect cases will be rare. However, when there's more than one
> sensible behavior, it's not unreasonable to pick one that is easier to
> implement or cheaper or whatever, and that's the category into which I
> would put this decision.
> 

OK, makes sense. I withdraw my objections to the original behavior, and
agree it's acceptable if it's reasonably documented.

regards

-- 
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: COPY FROM WHEN condition

[Robert Haas]

On Wed, Nov 28, 2018 at 6:17 PM Tomas Vondra
 wrote:
> > Comparing with overhead of setting snapshot before evaluating every row
> > and considering this
> >
> > kind of usage is not frequent it seems to me the behavior is acceptable
>
> I'm not really buying the argument that this behavior is acceptable
> simply because using the feature like this will be uncommon. That seems
> like a rather weak reason to accept it.
>
> I however agree we don't want to make COPY less efficient, at least not
> unless absolutely necessary. But I think we can handle this simply by
> restricting what's allowed to appear the FILTER clause.
>
> It should be fine to allow IMMUTABLE and STABLE functions, but not
> VOLATILE ones. That should fix the example I shared, because f() would
> not be allowed.

I don't think that's a very good solution.  It's perfectly sensible
for someone to want to do WHERE/FILTER random() < 0.01 to load a
smattering of rows, and this would rule that out for no very good
reason.

I think it would be fine to just document that if the filter condition
examines the state of the database, it will not see the results of the
COPY which is in progress.  I'm pretty sure there are other cases -
for example with triggers - where you can get code to run that can't
see the results of the command currently in progress, so I don't
really buy the idea that having a feature which works that way is
categorically unacceptable.

I agree that we can't justify flagrantly wrong behavior on the grounds
that correct behavior is expensive or on the grounds that the
incorrect cases will be rare. However, when there's more than one
sensible behavior, it's not unreasonable to pick one that is easier to
implement or cheaper or whatever, and that's the category into which I
would put this decision.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: COPY FROM WHEN condition

[Tomas Vondra]

On 12/4/18 10:44 AM, Alvaro Herrera wrote:
> After reading this thread, I think I like WHERE better than FILTER.
> Tally:
> 
> WHERE: Adam Berlin, Lim Myungkyu, Dean Rasheed, yours truly
> FILTER: Tomas Vondra, Surafel Temesgen
> 
> Couldn't find others expressing an opinion in this regard.
> 

While I still like FILTER more, I won't object to using WHERE if others
thinks it's a better choice.

> On 2018-Nov-30, Tomas Vondra wrote:
> 
>> I think it should be enough just to switch to CIM_SINGLE and
>> increment the command counter after each inserted row.
> 
> Do we apply command counter increment per row with some other COPY 
> option?

I don't think we increment the command counter anywhere, most likely
because COPY is not allowed to run any queries directly so far.

> Per-row CCI makes me a bit uncomfortable because with you'd get in
> trouble with a large copy.  I think it's particularly nasty here,
> precisely because you may want to filter out some rows of a very
> large file, and the CCI may prevent that from working.
Sure.

> I'm not convinced by the example case of reading how many tuples
> you've imported so far in the WHERE/WHEN/FILTER clause each time
> (that'd become incrementally slower as it progresses).
> 

Well, not sure how else am I supposed to convince you? It's an example
of a behavior that's IMHO surprising and inconsistent with things that
might be reasonably expected to behave similarly. It may not be a
perfect example, but that's the price for simplicity.

FWIW, another way to achieve mostly the same filtering feature is a
BEFORE INSERT trigger:

  create or replace function copy_filter() returns trigger as $$
  declare
v_c int;
  begin
select count(*) into v_c from t;
if v_c >= 100 then
  return null;
end if;
return NEW;
  end; $$ language plpgsql;

  create trigger filter before insert on t
 for each row execute procedure copy_filter();

This behaves consistently with INSERT, i.e. it enforces the total count
constraint the same way. And the COPY FILTER behaves differently.

FWIW I do realize this is not a particularly great check - for example,
it will not see effects of concurrent transactions etc. All I'm saying
is I find it annoying/strange that it behaves differently.

Also, considering the trigger does the right thing, maybe I spoke too
early about the command counter not being incremented?

regards

-- 
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: COPY FROM WHEN condition

[Alvaro Herrera]

After reading this thread, I think I like WHERE better than FILTER.
Tally:

WHERE: Adam Berlin, Lim Myungkyu, Dean Rasheed, yours truly
FILTER: Tomas Vondra, Surafel Temesgen

Couldn't find others expressing an opinion in this regard.

On 2018-Nov-30, Tomas Vondra wrote:

> I think it should be enough just to switch to CIM_SINGLE and increment the
> command counter after each inserted row.

Do we apply command counter increment per row with some other COPY
option?  Per-row CCI makes me a bit uncomfortable because with you'd get
in trouble with a large copy.  I think it's particularly nasty here,
precisely because you may want to filter out some rows of a very large
file, and the CCI may prevent that from working.
I'm not convinced by the example case of reading how many tuples you've
imported so far in the WHERE/WHEN/FILTER clause each time (that'd become
incrementally slower as it progresses).

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: COPY FROM WHEN condition

[Tomas Vondra]

On 11/30/18 2:00 PM, Surafel Temesgen wrote:



On Thu, Nov 29, 2018 at 2:17 AM Tomas Vondra 
mailto:tomas.von...@2ndquadrant.com>> wrote:


(c) allow VOLATILE functions in the FILTER clause, but change the
behavior to make the behavior sane

  Did changing the behavior means getting new snapshot before evaluating 
a tuple to ensure the function sees results of any previously executed 
queries or there are   other mechanism that can make the behavior sane?




I think it should be enough just to switch to CIM_SINGLE and increment 
the command counter after each inserted row.



Which leaves us with (b) and (c). Clearly, (b) is simpler to implement,
because it (c) needs to do the detection too, and then some additional
stuff. I'm not sure how much more complex (c) is, compared to (b).

The attache patch implement option b prohibit VOLATILE functions but i 
am open to change


OK. I'll take a look.


regards

--
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: COPY FROM WHEN condition

[Surafel Temesgen]

On Thu, Nov 29, 2018 at 2:17 AM Tomas Vondra 
wrote:

(c) allow VOLATILE functions in the FILTER clause, but change the
> behavior to make the behavior sane
>

 Did changing the behavior means getting new snapshot before evaluating a
tuple to ensure the function sees results of any previously executed
queries or there are   other mechanism that can make the behavior sane?

Which leaves us with (b) and (c). Clearly, (b) is simpler to implement,
> because it (c) needs to do the detection too, and then some additional
> stuff. I'm not sure how much more complex (c) is, compared to (b).
>
> The attache patch implement option b prohibit VOLATILE functions but i am
open to change
regards
Surafel
diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
index 411941ed31..15b6ddebed 100644
--- a/doc/src/sgml/ref/copy.sgml
+++ b/doc/src/sgml/ref/copy.sgml
@@ -25,6 +25,7 @@ PostgreSQL documentation
 COPY table_name [ ( column_name [, ...] ) ]
 FROM { 'filename' | PROGRAM 'command' | STDIN }
 [ [ WITH ] ( option [, ...] ) ]
+[ FILTER ( condition ) ]
 
 COPY { table_name [ ( column_name [, ...] ) ] | ( query ) }
 TO { 'filename' | PROGRAM 'command' | STDOUT }
@@ -353,6 +354,30 @@ COPY { table_name [ ( 

 
+   
+FILTER
+
+   
+The optional FILTER clause has the general form
+
+FILTER condition
+
+where condition is
+any expression that evaluates to a result of type
+boolean.  Any row that does not satisfy this
+condition will not be inserted to the table.  A row satisfies the
+condition if it returns true when the actual row values are
+substituted for any variable references.
+   
+
+   
+Currently, FILTER expressions cannot contain
+subqueries.
+   
+
+
+   
+
   
  
 
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 4aa8890fe8..b17d84e3fc 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -40,7 +40,11 @@
 #include "miscadmin.h"
 #include "optimizer/clauses.h"
 #include "optimizer/planner.h"
+#include "optimizer/prep.h"
 #include "nodes/makefuncs.h"
+#include "parser/parse_coerce.h"
+#include "parser/parse_collate.h"
+#include "parser/parse_expr.h"
 #include "parser/parse_relation.h"
 #include "port/pg_bswap.h"
 #include "rewrite/rewriteHandler.h"
@@ -150,6 +154,7 @@ typedef struct CopyStateData
 	bool		convert_selectively;	/* do selective binary conversion? */
 	List	   *convert_select; /* list of column names (can be NIL) */
 	bool	   *convert_select_flags;	/* per-column CSV/TEXT CS flags */
+	Node	   *filterClause;	/* FILTER condition (or NULL) */
 
 	/* these are just for error messages, see CopyFromErrorCallback */
 	const char *cur_relname;	/* table name for error messages */
@@ -180,6 +185,7 @@ typedef struct CopyStateData
 	ExprState **defexprs;		/* array of default att expressions */
 	bool		volatile_defexprs;	/* is any of defexprs volatile? */
 	List	   *range_table;
+	ExprState  *qualexpr;
 
 	TransitionCaptureState *transition_capture;
 
@@ -801,6 +807,7 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt,
 	Relation	rel;
 	Oid			relid;
 	RawStmt*query = NULL;
+	Node*filterClause = NULL;
 
 	/*
 	 * Disallow COPY to/from file or program except to users with the
@@ -854,6 +861,30 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt,
 			NULL, false, false);
 		rte->requiredPerms = (is_from ? ACL_INSERT : ACL_SELECT);
 
+		if (stmt->filterClause)
+		{
+			/* add rte to column namespace  */
+			addRTEtoQuery(pstate, rte, false, true, true);
+
+			/* Transform the raw expression tree */
+			filterClause = transformExpr(pstate, stmt->filterClause, EXPR_KIND_COPY_FILTER);
+
+			if (contain_volatile_functions(filterClause))
+ereport(ERROR,
+		(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+		 errmsg("function in FILTER clause should not be volatile")));
+
+			/*  Make sure it yields a boolean result. */
+			filterClause = coerce_to_boolean(pstate, filterClause, "FILTER");
+			/* we have to fix its collations too */
+			assign_expr_collations(pstate, filterClause);
+
+			filterClause = eval_const_expressions(NULL, filterClause);
+
+			filterClause = (Node *) canonicalize_qual((Expr *) filterClause, false);
+			filterClause = (Node *) make_ands_implicit((Expr *) filterClause);
+		}
+
 		tupDesc = RelationGetDescr(rel);
 		attnums = CopyGetAttnums(tupDesc, rel, stmt->attlist);
 		foreach(cur, attnums)
@@ -1002,6 +1033,7 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt,
 
 		cstate = BeginCopyFrom(pstate, rel, stmt->filename, stmt->is_program,
 			   NULL, stmt->attlist, stmt->options);
+		cstate->filterClause = filterClause;
 		*processed = CopyFrom(cstate);	/* copy from file to database */
 		EndCopyFrom(cstate);
 	}
@@ -2531,6 +2563,10 @@ CopyFrom(CopyState cstate)
 	if (cstate->rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
 		proute = ExecSetupPartitionTupleRouting(NULL, cstate->rel);
 
+	if (cstate->filterClause)
+		cstate->qualexpr = 

Re: COPY FROM WHEN condition

[Tomas Vondra]

On 11/26/18 2:25 PM, Surafel Temesgen wrote:
> 
> 
> On Sat, Nov 24, 2018 at 5:09 AM Tomas Vondra
> mailto:tomas.von...@2ndquadrant.com>> wrote:
> 
> 
> 1) While comparing this to the FILTER clause we already have for
> aggregates, I've noticed the aggregate version is
> 
>     FILTER '(' WHERE a_expr ')'
> 
> while here we have
> 
>     FILTER '(' a_expr ')'
> 
> For a while I was thinking that maybe we should use the same syntax
> here, but I don't think so. The WHERE bit seems rather unnecessary and
> we probably implemented it only because it's required by SQL standard,
> which does not apply to COPY. So I think this is fine.
> 
>  
> ok
> 
> 
> 2) The various parser checks emit errors like this:
> 
>     case EXPR_KIND_COPY_FILTER:
>         err = _("cannot use subquery in copy from FILTER condition");
>         break;
> 
> I think the "copy from" should be capitalized, to make it clear that it
> refers to a COPY FROM command and not a copy of something.
> 
> 
> 3) I think there should be regression tests for these prohibited things,
> i.e. for a set-returning function, for a non-existent column, etc.
> 
> 
> fixed
> 
> 
> 4) What might be somewhat confusing for users is that the filter uses a
> single snapshot to evaluate the conditions for all rows. That is, one
> might do this
> 
>     create or replace function f() returns int as $$
>         select count(*)::int from t;
>     $$ language sql;
> 
> and hope that
> 
>     copy t from '/...' filter (f() <= 100);
> 
> only ever imports the first 100 rows - but that's not true, of course,
> because f() uses the snapshot acquired at the very beginning. For
> example INSERT SELECT does behave differently:
> 
>     test=# copy t from '/home/user/t.data' filter (f() < 100);
>     COPY 81
>     test=# insert into t select * from t where f() < 100;
>     INSERT 0 19
> 
> Obviously, this is not an issue when the filter clause references only
> the input row (which I assume will be the primary use case).
> 
> Not sure if this is expected / appropriate behavior, and if the patch
> needs to do something else here.
> 
> Comparing with overhead of setting snapshot before evaluating every row
> and considering this
> 
> kind of usage is not frequent it seems to me the behavior is acceptable
> 

I'm not really buying the argument that this behavior is acceptable
simply because using the feature like this will be uncommon. That seems
like a rather weak reason to accept it.

I however agree we don't want to make COPY less efficient, at least not
unless absolutely necessary. But I think we can handle this simply by
restricting what's allowed to appear the FILTER clause.

It should be fine to allow IMMUTABLE and STABLE functions, but not
VOLATILE ones. That should fix the example I shared, because f() would
not be allowed.

We could mark is as STABLE explicitly, which would make it usable in the
FILTER clause, but STABLE says "same result for all calls in the
statement" so the behavior would be expected and essentially legal (in a
way it'd be mislabeling, but we trust it on other places too).

So I think there are four options:

(a) accept the current behavior (single snapshot, same result for all
function calls)

(b) prohibit VOLATILE functions in the FILTER clause altogether

(c) allow VOLATILE functions in the FILTER clause, but change the
behavior to make the behavior sane


I definitely vote -1 on (a) unless someone presents a much better
argument for allowing it than "it's uncommon".

Which leaves us with (b) and (c). Clearly, (b) is simpler to implement,
because it (c) needs to do the detection too, and then some additional
stuff. I'm not sure how much more complex (c) is, compared to (b).

After eyeballing the copy.c code for a bit, I think it would need to use
CIM_SINGLE when there are volatile functions, similarly to volatile
default values (see volatile_defexprs), and then increment the command
ID after each insert. Of course, we don't want to do this by default,
but only when actually needed (with VOLATILE functions), because the
multi-inserts are quite a bit more efficient.


regards

-- 
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: COPY FROM WHEN condition

[Surafel Temesgen]

On Sat, Nov 24, 2018 at 5:09 AM Tomas Vondra 
wrote:

>
> 1) While comparing this to the FILTER clause we already have for
> aggregates, I've noticed the aggregate version is
>
> FILTER '(' WHERE a_expr ')'
>
> while here we have
>
> FILTER '(' a_expr ')'
>
> For a while I was thinking that maybe we should use the same syntax
> here, but I don't think so. The WHERE bit seems rather unnecessary and
> we probably implemented it only because it's required by SQL standard,
> which does not apply to COPY. So I think this is fine.


ok

>
> 2) The various parser checks emit errors like this:
>
> case EXPR_KIND_COPY_FILTER:
> err = _("cannot use subquery in copy from FILTER condition");
> break;
>
> I think the "copy from" should be capitalized, to make it clear that it
> refers to a COPY FROM command and not a copy of something.
>
>
> 3) I think there should be regression tests for these prohibited things,
> i.e. for a set-returning function, for a non-existent column, etc.
>
>
fixed

>
> 4) What might be somewhat confusing for users is that the filter uses a
> single snapshot to evaluate the conditions for all rows. That is, one
> might do this
>
> create or replace function f() returns int as $$
> select count(*)::int from t;
> $$ language sql;
>
> and hope that
>
> copy t from '/...' filter (f() <= 100);
>
> only ever imports the first 100 rows - but that's not true, of course,
> because f() uses the snapshot acquired at the very beginning. For
> example INSERT SELECT does behave differently:
>
> test=# copy t from '/home/user/t.data' filter (f() < 100);
> COPY 81
> test=# insert into t select * from t where f() < 100;
> INSERT 0 19
>
> Obviously, this is not an issue when the filter clause references only
> the input row (which I assume will be the primary use case).
>
> Not sure if this is expected / appropriate behavior, and if the patch
> needs to do something else here.
>
> Comparing with overhead of setting snapshot before evaluating every row
and considering this

kind of usage is not frequent it seems to me the behavior is acceptable

regards
Surafel
diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
index 411941ed31..15b6ddebed 100644
--- a/doc/src/sgml/ref/copy.sgml
+++ b/doc/src/sgml/ref/copy.sgml
@@ -25,6 +25,7 @@ PostgreSQL documentation
 COPY table_name [ ( column_name [, ...] ) ]
 FROM { 'filename' | PROGRAM 'command' | STDIN }
 [ [ WITH ] ( option [, ...] ) ]
+[ FILTER ( condition ) ]
 
 COPY { table_name [ ( column_name [, ...] ) ] | ( query ) }
 TO { 'filename' | PROGRAM 'command' | STDOUT }
@@ -353,6 +354,30 @@ COPY { table_name [ ( 

 
+   
+FILTER
+
+   
+The optional FILTER clause has the general form
+
+FILTER condition
+
+where condition is
+any expression that evaluates to a result of type
+boolean.  Any row that does not satisfy this
+condition will not be inserted to the table.  A row satisfies the
+condition if it returns true when the actual row values are
+substituted for any variable references.
+   
+
+   
+Currently, FILTER expressions cannot contain
+subqueries.
+   
+
+
+   
+
   
  
 
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 4aa8890fe8..bb260c41fd 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -40,7 +40,11 @@
 #include "miscadmin.h"
 #include "optimizer/clauses.h"
 #include "optimizer/planner.h"
+#include "optimizer/prep.h"
 #include "nodes/makefuncs.h"
+#include "parser/parse_coerce.h"
+#include "parser/parse_collate.h"
+#include "parser/parse_expr.h"
 #include "parser/parse_relation.h"
 #include "port/pg_bswap.h"
 #include "rewrite/rewriteHandler.h"
@@ -150,6 +154,7 @@ typedef struct CopyStateData
 	bool		convert_selectively;	/* do selective binary conversion? */
 	List	   *convert_select; /* list of column names (can be NIL) */
 	bool	   *convert_select_flags;	/* per-column CSV/TEXT CS flags */
+	Node	   *filterClause;	/* FILTER condition (or NULL) */
 
 	/* these are just for error messages, see CopyFromErrorCallback */
 	const char *cur_relname;	/* table name for error messages */
@@ -180,6 +185,7 @@ typedef struct CopyStateData
 	ExprState **defexprs;		/* array of default att expressions */
 	bool		volatile_defexprs;	/* is any of defexprs volatile? */
 	List	   *range_table;
+	ExprState  *qualexpr;
 
 	TransitionCaptureState *transition_capture;
 
@@ -801,6 +807,7 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt,
 	Relation	rel;
 	Oid			relid;
 	RawStmt*query = NULL;
+	Node*filterClause = NULL;
 
 	/*
 	 * Disallow COPY to/from file or program except to users with the
@@ -854,6 +861,25 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt,
 			NULL, false, false);
 		rte->requiredPerms = (is_from ? ACL_INSERT : ACL_SELECT);
 
+		if (stmt->filterClause)
+		{
+			/* add rte to column namespace  */
+			addRTEtoQuery(pstate, rte, false, true, 

Re: COPY FROM WHEN condition

[Surafel Temesgen]

On Sat, Nov 24, 2018 at 12:02 PM Dean Rasheed 
wrote:

> Right now we have 2 syntaxes for filtering rows in queries, both of
> which use WHERE immediately before the condition:
>
> 1). SELECT ... FROM ... WHERE condition
>
> 2). SELECT agg_fn FILTER (WHERE condition) FROM ...
>
> I'm not a huge fan of (2), but that's the SQL standard, so we're stuck
> with it. There's a certain consistency in it's use of WHERE to
> introduce the condition, and preceding that with FILTER helps to
> distinguish it from any later WHERE clause. But what you'd be adding
> here would be a 3rd syntax
>
> 3). COPY ... FROM ... FILTER condition
>
> which IMO will just lead to confusion.
>


your case is for retrieving data but this is for deciding which data to
insert and word FILTER I think describe it more and not lead to confusion.

regards
Surafel

Re: COPY FROM WHEN condition

[Dean Rasheed]

On Sat, 24 Nov 2018 at 02:09, Tomas Vondra  wrote:
> On 11/23/18 12:14 PM, Surafel Temesgen wrote:
> > On Sun, Nov 11, 2018 at 11:59 PM Tomas Vondra
> > mailto:tomas.von...@2ndquadrant.com>> wrote:
> > So, what about using FILTER here? We already use it for aggregates when
> > filtering rows to process.
> >
> > i think its good idea and describe its purpose more. Attache is a
> > patch that use FILTER instead
>

FWIW, I vote for just using WHERE here.

Right now we have 2 syntaxes for filtering rows in queries, both of
which use WHERE immediately before the condition:

1). SELECT ... FROM ... WHERE condition

2). SELECT agg_fn FILTER (WHERE condition) FROM ...

I'm not a huge fan of (2), but that's the SQL standard, so we're stuck
with it. There's a certain consistency in it's use of WHERE to
introduce the condition, and preceding that with FILTER helps to
distinguish it from any later WHERE clause. But what you'd be adding
here would be a 3rd syntax

3). COPY ... FROM ... FILTER condition

which IMO will just lead to confusion.

Regards,
Dean

Re: COPY FROM WHEN condition

[Tomas Vondra]

On 11/23/18 12:14 PM, Surafel Temesgen wrote:
> 
> 
> On Sun, Nov 11, 2018 at 11:59 PM Tomas Vondra
> mailto:tomas.von...@2ndquadrant.com>> wrote:
> 
> 
> So, what about using FILTER here? We already use it for aggregates when
> filtering rows to process.
> 
> i think its good idea and describe its purpose more. Attache is a
> patch that use FILTER instead

Thanks, looks good to me. A couple of minor points:

1) While comparing this to the FILTER clause we already have for
aggregates, I've noticed the aggregate version is

FILTER '(' WHERE a_expr ')'

while here we have

FILTER '(' a_expr ')'

For a while I was thinking that maybe we should use the same syntax
here, but I don't think so. The WHERE bit seems rather unnecessary and
we probably implemented it only because it's required by SQL standard,
which does not apply to COPY. So I think this is fine.


2) The various parser checks emit errors like this:

case EXPR_KIND_COPY_FILTER:
err = _("cannot use subquery in copy from FILTER condition");
break;

I think the "copy from" should be capitalized, to make it clear that it
refers to a COPY FROM command and not a copy of something.


3) I think there should be regression tests for these prohibited things,
i.e. for a set-returning function, for a non-existent column, etc.


4) What might be somewhat confusing for users is that the filter uses a
single snapshot to evaluate the conditions for all rows. That is, one
might do this

create or replace function f() returns int as $$
select count(*)::int from t;
$$ language sql;

and hope that

copy t from '/...' filter (f() <= 100);

only ever imports the first 100 rows - but that's not true, of course,
because f() uses the snapshot acquired at the very beginning. For
example INSERT SELECT does behave differently:

test=# copy t from '/home/user/t.data' filter (f() < 100);
COPY 81
test=# insert into t select * from t where f() < 100;
INSERT 0 19

Obviously, this is not an issue when the filter clause references only
the input row (which I assume will be the primary use case).

Not sure if this is expected / appropriate behavior, and if the patch
needs to do something else here.


regards

-- 
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: COPY FROM WHEN condition

[Surafel Temesgen]

On Sun, Nov 11, 2018 at 11:59 PM Tomas Vondra 
wrote:

>
> So, what about using FILTER here? We already use it for aggregates when
> filtering rows to process.
>
> i think its good idea and describe its purpose more. Attache is a patch
that use FILTER instead
regards
Surafel
diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
index 411941ed31..15b6ddebed 100644
--- a/doc/src/sgml/ref/copy.sgml
+++ b/doc/src/sgml/ref/copy.sgml
@@ -25,6 +25,7 @@ PostgreSQL documentation
 COPY table_name [ ( column_name [, ...] ) ]
 FROM { 'filename' | PROGRAM 'command' | STDIN }
 [ [ WITH ] ( option [, ...] ) ]
+[ FILTER ( condition ) ]
 
 COPY { table_name [ ( column_name [, ...] ) ] | ( query ) }
 TO { 'filename' | PROGRAM 'command' | STDOUT }
@@ -353,6 +354,30 @@ COPY { table_name [ ( 

 
+   
+FILTER
+
+   
+The optional FILTER clause has the general form
+
+FILTER condition
+
+where condition is
+any expression that evaluates to a result of type
+boolean.  Any row that does not satisfy this
+condition will not be inserted to the table.  A row satisfies the
+condition if it returns true when the actual row values are
+substituted for any variable references.
+   
+
+   
+Currently, FILTER expressions cannot contain
+subqueries.
+   
+
+
+   
+
   
  
 
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 4aa8890fe8..bb260c41fd 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -40,7 +40,11 @@
 #include "miscadmin.h"
 #include "optimizer/clauses.h"
 #include "optimizer/planner.h"
+#include "optimizer/prep.h"
 #include "nodes/makefuncs.h"
+#include "parser/parse_coerce.h"
+#include "parser/parse_collate.h"
+#include "parser/parse_expr.h"
 #include "parser/parse_relation.h"
 #include "port/pg_bswap.h"
 #include "rewrite/rewriteHandler.h"
@@ -150,6 +154,7 @@ typedef struct CopyStateData
 	bool		convert_selectively;	/* do selective binary conversion? */
 	List	   *convert_select; /* list of column names (can be NIL) */
 	bool	   *convert_select_flags;	/* per-column CSV/TEXT CS flags */
+	Node	   *filterClause;	/* FILTER condition (or NULL) */
 
 	/* these are just for error messages, see CopyFromErrorCallback */
 	const char *cur_relname;	/* table name for error messages */
@@ -180,6 +185,7 @@ typedef struct CopyStateData
 	ExprState **defexprs;		/* array of default att expressions */
 	bool		volatile_defexprs;	/* is any of defexprs volatile? */
 	List	   *range_table;
+	ExprState  *qualexpr;
 
 	TransitionCaptureState *transition_capture;
 
@@ -801,6 +807,7 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt,
 	Relation	rel;
 	Oid			relid;
 	RawStmt*query = NULL;
+	Node*filterClause = NULL;
 
 	/*
 	 * Disallow COPY to/from file or program except to users with the
@@ -854,6 +861,25 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt,
 			NULL, false, false);
 		rte->requiredPerms = (is_from ? ACL_INSERT : ACL_SELECT);
 
+		if (stmt->filterClause)
+		{
+			/* add rte to column namespace  */
+			addRTEtoQuery(pstate, rte, false, true, true);
+
+			/* Transform the raw expression tree */
+			filterClause = transformExpr(pstate, stmt->filterClause, EXPR_KIND_COPY_FILTER);
+
+			/*  Make sure it yields a boolean result. */
+			filterClause = coerce_to_boolean(pstate, filterClause, "FILTER");
+			/* we have to fix its collations too */
+			assign_expr_collations(pstate, filterClause);
+
+			filterClause = eval_const_expressions(NULL, filterClause);
+
+			filterClause = (Node *) canonicalize_qual((Expr *) filterClause, false);
+			filterClause = (Node *) make_ands_implicit((Expr *) filterClause);
+		}
+
 		tupDesc = RelationGetDescr(rel);
 		attnums = CopyGetAttnums(tupDesc, rel, stmt->attlist);
 		foreach(cur, attnums)
@@ -1002,6 +1028,7 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt,
 
 		cstate = BeginCopyFrom(pstate, rel, stmt->filename, stmt->is_program,
 			   NULL, stmt->attlist, stmt->options);
+		cstate->filterClause = filterClause;
 		*processed = CopyFrom(cstate);	/* copy from file to database */
 		EndCopyFrom(cstate);
 	}
@@ -2531,6 +2558,10 @@ CopyFrom(CopyState cstate)
 	if (cstate->rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
 		proute = ExecSetupPartitionTupleRouting(NULL, cstate->rel);
 
+	if (cstate->filterClause)
+		cstate->qualexpr = ExecInitQual(castNode(List, cstate->filterClause),
+>ps);
+
 	/*
 	 * It's more efficient to prepare a bunch of tuples for insertion, and
 	 * insert them in one heap_multi_insert() call, than call heap_insert()
@@ -2679,6 +2710,13 @@ CopyFrom(CopyState cstate)
 		slot = myslot;
 		ExecStoreHeapTuple(tuple, slot, false);
 
+		if (cstate->filterClause)
+		{
+			econtext->ecxt_scantuple = myslot;
+			if (!ExecQual(cstate->qualexpr, econtext))
+continue;
+		}
+
 		/* Determine the partition to heap_insert the tuple into */
 		if (proute)
 		{
diff --git a/src/backend/nodes/copyfuncs.c 

RE: COPY FROM WHEN condition

[myungkyu.lim]

>> COPY table_name WHERE (some_condition)
>> 
>> Users should already be familiar with the idea that WHERE performs a filter.
>> 

> So, what about using FILTER here? We already use it for aggregates when
> filtering rows to process.

> That being said, I have no strong feelings either way. I'd be OK with
> both WHEN and WHERE.

I don't think it's an important point,

In gram.y,
where_clause:
WHERE a_expr
{ $$ = $2; }
| /*EMPTY*/ 
{ $$ = NULL; }
;
This is similar to the 'opt_when_clause' in this patch.

So, I think 'WHERE' is a better form.

BTW, 3rd patch worked very well in my tests.
However, some wrong code style still exists.

Node*whenClause= NULL;
cstate->whenClause=whenClause;

Best regards,
Myungkyu, Lim

Re: COPY FROM WHEN condition

[Tomas Vondra]

On 11/9/18 4:51 PM, Adam Berlin wrote:
> As a newcomer to this patch, when I read this example:
> 
> COPY table_name WHEN (some_condition)
> 
> .. I expect COPY to only be run when the condition is true, and I do
not expect the WHEN clause to filter rows. I'm curious what you think about:
> 

Hmmm. Currently we have WHEN clause in three places:

1) triggers, where it specifies arbitrary expression on rows,
determining whether the trigger should be invoked at all

2) event triggers, where it does about the same thing as for regular
triggers, but only expressions "column IN (...)" are allowed

3) CASE WHEN ... THEN  ...

I'd say 3 is rather unrelated to this discussion, and 1+2 seem to
support you objection to using WHEN for COPY, as it kinda specifies
whether the command itself should be executed.

> COPY table_name WHERE (some_condition)
> 
> Users should already be familiar with the idea that WHERE performs a filter.
> 

So, what about using FILTER here? We already use it for aggregates when
filtering rows to process.

That being said, I have no strong feelings either way. I'd be OK with
both WHEN and WHERE.

regards

-- 
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: COPY FROM WHEN condition

[Adam Berlin]

As a newcomer to this patch, when I read this example:

COPY table_name WHEN (some_condition)

.. I expect COPY to only be run when the condition is true, and I do not expect 
the WHEN clause to filter rows. I'm curious what you think about:

COPY table_name WHERE (some_condition)

Users should already be familiar with the idea that WHERE performs a filter.

Re: COPY FROM WHEN condition

[Daniel Verite]

David Fetter wrote:

> It also seems like a violation of separation of concerns to couple
> FEBE to grammar, so there'd need to be some way to do those things
> separately, too.

After re-reading psql/copy.c, I withdraw what I said upthread:
it doesn't appear necessary to add anything to support the WHEN
condition with \copy.

\copy does have a dedicated mini-parser, but it doesn't try to
recognize every option: it's only concerned with getting the bits of
information that are needed to perform the client-side work:
- whether it's a copy from or to
- what exact form and value has the 'filename' argument immediately
 after from or to:
 '' | PROGRAM '' | stdin | stdout | pstdout | pstdout

It doesn't really care what the options are, just where they are
in the buffer, so they can be copied into the COPY SQL statement.

From the code:
 * The documented syntax is:
 *  \copy tablename [(columnlist)] from|to filename [options]
 *  \copy ( query stmt ) to filename [options]

The WHEN clause would be part of the [options], which
are handled as simply as this in parse_slash_copy():

  /* Collect the rest of the line (COPY options) */
  token = strtokx(NULL, "", NULL, NULL,
  0, false, false, pset.encoding);
  if (token)
  result->after_tofrom = pg_strdup(token);

So unless there's something particular in the WHEN clause
expression that could make this strtokx() invocation error out,
this should work directly.

Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite

Re: COPY FROM WHEN condition

[Corey Huinker]

>
>
> > SELECT x.a, sum(x.b)
> > FROM ( COPY INLINE '/path/to/foo.txt' FORMAT CSV ) as x( a integer, b
> numeric, c text, d date, e json) )
>
> Apologies for bike-shedding, but wouldn't the following be a better
> fit with the current COPY?
>
> COPY t(a integer, b numeric, c text, d date, e json) FROM
> '/path/to/foo.txt' WITH (FORMAT CSV, INLINE)
>

+1 Very much a better fit.

>
>

Re: COPY FROM WHEN condition

[David Fetter]

On Fri, Nov 02, 2018 at 12:58:12PM +0100, Daniel Verite wrote:
>   Pavel Stehule wrote:
> 
> > > SELECT x.a, sum(x.b)
> > > FROM ( COPY INLINE '/path/to/foo.txt' FORMAT CSV ) as x( a integer, b
> > > numeric, c text, d date, e json) )
> > > WHERE x.d >= '2018-11-01'
> > >
> > >
> > Without some special feature this example is not extra useful. It is based
> > on copy on server that can use only super user with full rights.
> 
> And if superuser, one might use the file data wrapper [1]  to get
> the same results without the need for the explicit COPY in the query.
> 
> BTW, this brings into question whether the [WHEN condition] clause
> should be included in the options of file_fdw, as it supports pretty
> much all COPY options.
> 
> Also, psql's \copy should gain the option too?

tl;dr: \copy support is a very large can of worms.

psql's \copy is something which should probably be handled separately
from COPY, as it's both a way to access the filesystem without
superuser permission and an interface to the COPY part of the
protocol.  It seems like poor design to add grammar to support a
single client, so we'd need to think about this in terms of what we
want to support on the client side independent of specific clients. It
also seems like a violation of separation of concerns to couple FEBE
to grammar, so there'd need to be some way to do those things
separately, too.

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

Re: COPY FROM WHEN condition

[David Fetter]

On Thu, Nov 01, 2018 at 10:57:25PM -0400, Corey Huinker wrote:
> >
> > > Are you thinking something like having a COPY command that provides
> > > results in such a way that they could be referenced in a FROM clause
> > > (perhaps a COPY that defines a cursor…)?
> >
> > That would also be nice, but what I was thinking of was that some
> > highly restricted subset of cases of SQL in general could lend
> > themselves to levels of optimization that would be impractical in
> > other contexts.
> 
> If COPY (or a syntactical equivalent) can return a result set, then the
> whole of SQL is available to filter and aggregate the results and we don't
> have to invent new syntax, or endure confusion whenCOPY-WHEN syntax behaves
> subtly different from a similar FROM-WHERE.

That's an excellent point.

> Also, what would we be saving computationally? The whole file (or program
> output) has to be consumed no matter what, the columns have to be parsed no
> matter what. At least some of the columns have to be converted to their
> assigned datatypes enough to know whether or not to filter the row, but we
> might be able push that logic inside a copy. I'm thinking of something like
> this:
> 
> SELECT x.a, sum(x.b)
> FROM ( COPY INLINE '/path/to/foo.txt' FORMAT CSV ) as x( a integer, b 
> numeric, c text, d date, e json) )

Apologies for bike-shedding, but wouldn't the following be a better
fit with the current COPY?

COPY t(a integer, b numeric, c text, d date, e json) FROM 
'/path/to/foo.txt' WITH (FORMAT CSV, INLINE)

> WHERE x.d >= '2018-11-01'
> 
> 
> In this case, there is the *opportunity* to see the following optimizations:
> - columns c and e are never referenced, and need never be turned into a
> datum (though we might do so just to confirm that they conform to the data
> type)

That sounds like something that could go inside the WITH extension
I'm proposing above.

[STRICT_TYPE boolean DEFAULT true]?

This might not be something that has to be in version 1.

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

Re: COPY FROM WHEN condition

[Tomas Vondra]

On 11/02/2018 03:57 AM, Corey Huinker wrote:
> > Are you thinking something like having a COPY command that provides
> > results in such a way that they could be referenced in a FROM clause
> > (perhaps a COPY that defines a cursor…)?
> 
> That would also be nice, but what I was thinking of was that some
> highly restricted subset of cases of SQL in general could lend
> themselves to levels of optimization that would be impractical in
> other contexts.
> 
> 
> If COPY (or a syntactical equivalent) can return a result set, then the
> whole of SQL is available to filter and aggregate the results and we
> don't have to invent new syntax, or endure confusion whenCOPY-WHEN
> syntax behaves subtly different from a similar FROM-WHERE.
> 
> Also, what would we be saving computationally? The whole file (or
> program output) has to be consumed no matter what, the columns have to
> be parsed no matter what. At least some of the columns have to be
> converted to their assigned datatypes enough to know whether or not to
> filter the row, but we might be able push that logic inside a copy. I'm
> thinking of something like this:
> 
> SELECT x.a, sum(x.b)
> FROM ( COPY INLINE '/path/to/foo.txt' FORMAT CSV ) as x( a integer,
> b numeric, c text, d date, e json) )
> WHERE x.d >= '2018-11-01'
> 
> 
> In this case, there is the /opportunity/ to see the following optimizations:
> - columns c and e are never referenced, and need never be turned into a
> datum (though we might do so just to confirm that they conform to the
> data type)
> - if column d is converted first, we can filter on it and avoid
> converting columns a,b
> - whatever optimizations we can infer from knowing that the two
> surviving columns will go directly into an aggregate
> 
> If we go this route, we can train the planner to notice other
> optimizations and add those mechanisms at that time, and then existing
> code gets faster.
> 
> If we go the COPY-WHEN route, then we have to make up new syntax for
> every possible future optimization.

IMHO those two things address vastly different use-cases. The COPY WHEN
case deals with filtering data while importing them into a database,
while what you're describing seems to be more about querying data stored
in a CSV file. But we already do have a solution for that - FDW, and I'd
say it's working pretty well. And AFAIK it does give you tools to
implement most of what you're asking for. I don't see why should we bolt
this on top of COPY, or how is it an alternative to COPY WHEN.

regards

-- 
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: COPY FROM WHEN condition

[Daniel Verite]

Pavel Stehule wrote:

> > SELECT x.a, sum(x.b)
> > FROM ( COPY INLINE '/path/to/foo.txt' FORMAT CSV ) as x( a integer, b
> > numeric, c text, d date, e json) )
> > WHERE x.d >= '2018-11-01'
> >
> >
> Without some special feature this example is not extra useful. It is based
> on copy on server that can use only super user with full rights.

And if superuser, one might use the file data wrapper [1]  to get
the same results without the need for the explicit COPY in the query.

BTW, this brings into question whether the [WHEN condition] clause
should be included in the options of file_fdw, as it supports pretty
much all COPY options.

Also, psql's \copy should gain the option too?


[1] https://www.postgresql.org/docs/current/static/file-fdw.html


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite

Re: COPY FROM WHEN condition

[Surafel Temesgen]

hi,

On Wed, Oct 31, 2018 at 10:54 AM Masahiko Sawada 
wrote:

> On Tue, Oct 30, 2018 at 11:47 PM Surafel Temesgen 
> wrote:
>
> I've looked at this patch and tested.
>
> When I use a function returning string in WHEN clause I got the following
> error:
>
> =# copy test from '/tmp/aaa.csv' (format 'csv') when (lower(t) = 'hello');
> ERROR:  could not determine which collation to use for lower() function
> HINT:  Use the COLLATE clause to set the collation explicitly.
> CONTEXT:  COPY hoge, line 1: "1,hoge,2018-01-01"
>
> And then although I specified COLLATE I got an another error (127 =
> T_CollateExpr):
>
> =# copy test from '/tmp/aaa.csv' (format 'csv') when (lower(t) collate
> "en_US" = 'hello');
> ERROR:  unrecognized node type: 127
>
> This error doesn't happen if I put the similar condition in WHEN
> clause for triggers. I think the patch needs to produce a reasonable
> error message.
>
> The attached patch include a fix for it .can you confirm it
regards
Surafel
diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
index 13a8b68d95..f9350afcdb 100644
--- a/doc/src/sgml/ref/copy.sgml
+++ b/doc/src/sgml/ref/copy.sgml
@@ -25,6 +25,7 @@ PostgreSQL documentation
 COPY table_name [ ( column_name [, ...] ) ]
 FROM { 'filename' | PROGRAM 'command' | STDIN }
 [ [ WITH ] ( option [, ...] ) ]
+[ WHEN ( condition ) ]
 
 COPY { table_name [ ( column_name [, ...] ) ] | ( query ) }
 TO { 'filename' | PROGRAM 'command' | STDOUT }
@@ -364,6 +365,30 @@ COPY { table_name [ ( 

 
+   
+WHEN
+
+   
+The optional WHEN clause has the general form
+
+WHEN condition
+
+where condition is
+any expression that evaluates to a result of type
+boolean.  Any row that does not satisfy this
+condition will not be inserted to the table.  A row satisfies the
+condition if it returns true when the actual row values are
+substituted for any variable references.
+   
+
+   
+Currently, WHEN expressions cannot contain
+subqueries.
+   
+
+
+   
+
   
  
 
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 9bc67ce60f..5e6c1d6133 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -38,7 +38,11 @@
 #include "miscadmin.h"
 #include "optimizer/clauses.h"
 #include "optimizer/planner.h"
+#include "optimizer/prep.h"
 #include "nodes/makefuncs.h"
+#include "parser/parse_coerce.h"
+#include "parser/parse_collate.h"
+#include "parser/parse_expr.h"
 #include "parser/parse_relation.h"
 #include "port/pg_bswap.h"
 #include "rewrite/rewriteHandler.h"
@@ -147,6 +151,7 @@ typedef struct CopyStateData
 	bool		convert_selectively;	/* do selective binary conversion? */
 	List	   *convert_select; /* list of column names (can be NIL) */
 	bool	   *convert_select_flags;	/* per-column CSV/TEXT CS flags */
+	Node	   *whenClause;	/* WHEN condition (or NULL) */
 
 	/* these are just for error messages, see CopyFromErrorCallback */
 	const char *cur_relname;	/* table name for error messages */
@@ -178,6 +183,7 @@ typedef struct CopyStateData
 	ExprState **defexprs;		/* array of default att expressions */
 	bool		volatile_defexprs;	/* is any of defexprs volatile? */
 	List	   *range_table;
+	ExprState  *qualexpr;
 
 	TransitionCaptureState *transition_capture;
 
@@ -797,6 +803,7 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt,
 	Relation	rel;
 	Oid			relid;
 	RawStmt*query = NULL;
+	Node*whenClause= NULL;
 
 	/*
 	 * Disallow COPY to/from file or program except to users with the
@@ -849,6 +856,25 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt,
 		rte = addRangeTableEntryForRelation(pstate, rel, NULL, false, false);
 		rte->requiredPerms = (is_from ? ACL_INSERT : ACL_SELECT);
 
+		if (stmt->whenClause)
+		{
+			/* add rte to column namespace  */
+			addRTEtoQuery(pstate, rte, false, true, true);
+
+			/* Transform the raw expression tree */
+			whenClause = transformExpr(pstate, stmt->whenClause, EXPR_KIND_COPY_FROM_WHEN);
+
+			/*  Make sure it yields a boolean result. */
+			whenClause = coerce_to_boolean(pstate, whenClause, "WHEN");
+			/* we have to fix its collations too */
+			assign_expr_collations(pstate, whenClause);
+
+			whenClause = eval_const_expressions(NULL, whenClause);
+
+			whenClause = (Node *) canonicalize_qual((Expr *) whenClause, false);
+			whenClause = (Node *) make_ands_implicit((Expr *) whenClause);
+		}
+
 		tupDesc = RelationGetDescr(rel);
 		attnums = CopyGetAttnums(tupDesc, rel, stmt->attlist);
 		foreach(cur, attnums)
@@ -997,6 +1023,7 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt,
 
 		cstate = BeginCopyFrom(pstate, rel, stmt->filename, stmt->is_program,
 			   NULL, stmt->attlist, stmt->options);
+		cstate->whenClause=whenClause;
 		*processed = CopyFrom(cstate);	/* copy from file to database */
 		EndCopyFrom(cstate);
 	}
@@ -2534,6 +2561,10 @@ CopyFrom(CopyState cstate)
 			ExecSetupChildParentMapForLeaf(proute);
 	}
 
+	if (cstate->whenClause)
+		cstate->qualexpr = 

Re: COPY FROM WHEN condition

[Pavel Stehule]

pá 2. 11. 2018 v 3:57 odesílatel Corey Huinker 
napsal:

> > Are you thinking something like having a COPY command that provides
>> > results in such a way that they could be referenced in a FROM clause
>> > (perhaps a COPY that defines a cursor…)?
>>
>> That would also be nice, but what I was thinking of was that some
>> highly restricted subset of cases of SQL in general could lend
>> themselves to levels of optimization that would be impractical in
>> other contexts.
>>
>
> If COPY (or a syntactical equivalent) can return a result set, then the
> whole of SQL is available to filter and aggregate the results and we don't
> have to invent new syntax, or endure confusion whenCOPY-WHEN syntax behaves
> subtly different from a similar FROM-WHERE.
>
> Also, what would we be saving computationally? The whole file (or program
> output) has to be consumed no matter what, the columns have to be parsed no
> matter what. At least some of the columns have to be converted to their
> assigned datatypes enough to know whether or not to filter the row, but we
> might be able push that logic inside a copy. I'm thinking of something like
> this:
>
> SELECT x.a, sum(x.b)
> FROM ( COPY INLINE '/path/to/foo.txt' FORMAT CSV ) as x( a integer, b
> numeric, c text, d date, e json) )
> WHERE x.d >= '2018-11-01'
>
>
Without some special feature this example is not extra useful. It is based
on copy on server that can use only super user with full rights.

What should be benefit of this feature?

Regards

Pavel


> In this case, there is the *opportunity* to see the following
> optimizations:
> - columns c and e are never referenced, and need never be turned into a
> datum (though we might do so just to confirm that they conform to the data
> type)
> - if column d is converted first, we can filter on it and avoid converting
> columns a,b
> - whatever optimizations we can infer from knowing that the two surviving
> columns will go directly into an aggregate
>
> If we go this route, we can train the planner to notice other
> optimizations and add those mechanisms at that time, and then existing code
> gets faster.
>
> If we go the COPY-WHEN route, then we have to make up new syntax for every
> possible future optimization.
>

Re: COPY FROM WHEN condition

[Corey Huinker]

>
> > Are you thinking something like having a COPY command that provides
> > results in such a way that they could be referenced in a FROM clause
> > (perhaps a COPY that defines a cursor…)?
>
> That would also be nice, but what I was thinking of was that some
> highly restricted subset of cases of SQL in general could lend
> themselves to levels of optimization that would be impractical in
> other contexts.
>

If COPY (or a syntactical equivalent) can return a result set, then the
whole of SQL is available to filter and aggregate the results and we don't
have to invent new syntax, or endure confusion whenCOPY-WHEN syntax behaves
subtly different from a similar FROM-WHERE.

Also, what would we be saving computationally? The whole file (or program
output) has to be consumed no matter what, the columns have to be parsed no
matter what. At least some of the columns have to be converted to their
assigned datatypes enough to know whether or not to filter the row, but we
might be able push that logic inside a copy. I'm thinking of something like
this:

SELECT x.a, sum(x.b)
FROM ( COPY INLINE '/path/to/foo.txt' FORMAT CSV ) as x( a integer, b
numeric, c text, d date, e json) )
WHERE x.d >= '2018-11-01'


In this case, there is the *opportunity* to see the following optimizations:
- columns c and e are never referenced, and need never be turned into a
datum (though we might do so just to confirm that they conform to the data
type)
- if column d is converted first, we can filter on it and avoid converting
columns a,b
- whatever optimizations we can infer from knowing that the two surviving
columns will go directly into an aggregate

If we go this route, we can train the planner to notice other optimizations
and add those mechanisms at that time, and then existing code gets faster.

If we go the COPY-WHEN route, then we have to make up new syntax for every
possible future optimization.

Re: COPY FROM WHEN condition

[David Fetter]

On Wed, Oct 31, 2018 at 11:21:33PM +, Nasby, Jim wrote:
> On Oct 11, 2018, at 10:35 AM, David Fetter  wrote:
> > 
> >> It didn't get far, but you may want to take a look at a rejected patch for
> >> copy_srf() (set returning function)
> >> https://www.postgresql.org/message-id/CADkLM%3DdoeiWQX4AGtDNG4PsWfSXz3ai7kY%3DPZm3sUhsUeev9Bg%40mail.gmail.com
> >> https://commitfest.postgresql.org/12/869/
> >> 
> >> Having a set returning function gives you the full expressiveness of SQL,
> >> at the cost of an extra materialization step.
> > 
> > I wonder whether something JIT-like could elide this. A very
> > interesting subset of such WHEN clauses could be pretty
> > straight-forward to implement in a pretty efficient way.
> 
> Are you thinking something like having a COPY command that provides
> results in such a way that they could be referenced in a FROM clause
> (perhaps a COPY that defines a cursor…)?

That would also be nice, but what I was thinking of was that some
highly restricted subset of cases of SQL in general could lend
themselves to levels of optimization that would be impractical in
other contexts.

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

Re: COPY FROM WHEN condition

[Nasby, Jim]

On Oct 11, 2018, at 10:35 AM, David Fetter  wrote:
> 
>> It didn't get far, but you may want to take a look at a rejected patch for
>> copy_srf() (set returning function)
>> https://www.postgresql.org/message-id/CADkLM%3DdoeiWQX4AGtDNG4PsWfSXz3ai7kY%3DPZm3sUhsUeev9Bg%40mail.gmail.com
>> https://commitfest.postgresql.org/12/869/
>> 
>> Having a set returning function gives you the full expressiveness of SQL,
>> at the cost of an extra materialization step.
> 
> I wonder whether something JIT-like could elide this. A very
> interesting subset of such WHEN clauses could be pretty
> straight-forward to implement in a pretty efficient way.

Are you thinking something like having a COPY command that provides results in 
such a way that they could be referenced in a FROM clause (perhaps a COPY that 
defines a cursor…)?

Re: COPY FROM WHEN condition

[Masahiko Sawada]

On Tue, Oct 30, 2018 at 11:47 PM Surafel Temesgen  wrote:
>
>
> Hi,
> Thank you for looking at it .
> On Sun, Oct 28, 2018 at 7:19 PM Tomas Vondra  
> wrote:
>>
>>
>> 1) I think this deserves at least some regression tests. Plenty of tests
>> already use COPY, but there's no coverage for the new piece. So let's
>> add a new test suite, or maybe add a couple of tests into copy2.sql.
>>
>>
>> 2) In copy.sqml, the new item is defined like this
>>
>> WHEN Clause
>>
>> I suggest we use just WHEN, that's what
>> the other items do (see ENCODING).
>>
>> The other thing is that this does not say what expressions are allowed
>> in the WHEN clause. It seems pretty close to WHEN clause for triggers,
>> which e.g. mentions that subselects are not allowed. I'm pretty sure
>> that's true here too, because it fails like this (118 = T_SubLink):
>>
>> test=# copy t(a,b,c) from '/tmp/t.data' when ((select 1) < 10);
>> ERROR:  unrecognized node type: 118
>>
>> So, the patch needs to detect this, produce a reasonable error message
>> and document the limitations in copy.sqml, just like we do for CREATE
>> TRIGGER.
>
> fixed
>>
>>
>> 3) For COPY TO, the WHEN clause is accepted but ignored, leading to
>> confusing cases like this:
>>
>> test=# copy t(a,b,c) to '/tmp/t.data' when ((select 100) < 10);
>> COPY 151690
>>
>> So, it contains subselect, but unlike COPY FROM it does not fail
>> (because we never execute it). The fun part is that the expression is
>> logically false, so a user might expect it to filter rows, yet we copy
>> everything.
>>
>> IMHO we need to either error-out in these cases, complaining about WHEN
>> not being supported for COPY TO, or make it work (effectively treating
>> it as a simpler alternative to COPY (subselect) TO).
>
> English is not my first language but I chose error-out because WHEN condition 
> for COPY TO seems to me semantically incorrect
>>
>>
>> AFAICS we could just get rid of the extra when_cluase variable and mess
>> with the cstate->whenClause directly, depending on how (3) gets fixed.
>
> I did it this way because CopyState structure memory allocate and initialize 
> in BeginCopyFrom but the analysis done before it
>>
>>
>> 5) As I mentioned, the CREATE TRIGGER already has WHEN clause, but it
>> requires it to be 'WHEN (expr)'. I suggest we do the same thing here,
>> requiring the parentheses.
>>
>>
>> 6) The skip logic in CopyFrom() seems to be slightly wrong. It does
>> work, but the next_record label is defined after CHECK_FOR_INTERRUPTS()
>> so a COPY will not respond to Ctrl-C unless it finds a row matching the
>> WHEN condition. If you have a highly selective condition, that's a bit
>> inconvenient.
>>
>> It also skips
>>
>> MemoryContextSwitchTo(GetPerTupleMemoryContext(estate));
>>
>> so I wonder what the heap_form_tuple() right after the next_record label
>> will use for tuples right after a skipped one. I'd bet it'll use the
>> oldcontext (essentially the long-lived context), essentially making it
>> a memory leak.
>>
>> So I suggest to get rid of the next_record label, and use 'continue'
>> instead of the 'goto next_record'.
>>
> fixed
>>

I've looked at this patch and tested.

When I use a function returning string in WHEN clause I got the following error:

=# copy test from '/tmp/aaa.csv' (format 'csv') when (lower(t) = 'hello');
ERROR:  could not determine which collation to use for lower() function
HINT:  Use the COLLATE clause to set the collation explicitly.
CONTEXT:  COPY hoge, line 1: "1,hoge,2018-01-01"

And then although I specified COLLATE I got an another error (127 =
T_CollateExpr):

=# copy test from '/tmp/aaa.csv' (format 'csv') when (lower(t) collate
"en_US" = 'hello');
ERROR:  unrecognized node type: 127

This error doesn't happen if I put the similar condition in WHEN
clause for triggers. I think the patch needs to produce a reasonable
error message.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Re: COPY FROM WHEN condition

[Surafel Temesgen]

Hi,
Thank you for looking at it .
On Sun, Oct 28, 2018 at 7:19 PM Tomas Vondra 
wrote:

>
> 1) I think this deserves at least some regression tests. Plenty of tests
> already use COPY, but there's no coverage for the new piece. So let's
> add a new test suite, or maybe add a couple of tests into copy2.sql.
>
>
> 2) In copy.sqml, the new item is defined like this
>
> WHEN Clause
>
> I suggest we use just WHEN, that's what
> the other items do (see ENCODING).
>
> The other thing is that this does not say what expressions are allowed
> in the WHEN clause. It seems pretty close to WHEN clause for triggers,
> which e.g. mentions that subselects are not allowed. I'm pretty sure
> that's true here too, because it fails like this (118 = T_SubLink):
>
> test=# copy t(a,b,c) from '/tmp/t.data' when ((select 1) < 10);
> ERROR:  unrecognized node type: 118
>
> So, the patch needs to detect this, produce a reasonable error message
> and document the limitations in copy.sqml, just like we do for CREATE
> TRIGGER.
>
fixed

>
> 3) For COPY TO, the WHEN clause is accepted but ignored, leading to
> confusing cases like this:
>
> test=# copy t(a,b,c) to '/tmp/t.data' when ((select 100) < 10);
> COPY 151690
>
> So, it contains subselect, but unlike COPY FROM it does not fail
> (because we never execute it). The fun part is that the expression is
> logically false, so a user might expect it to filter rows, yet we copy
> everything.
>
> IMHO we need to either error-out in these cases, complaining about WHEN
> not being supported for COPY TO, or make it work (effectively treating
> it as a simpler alternative to COPY (subselect) TO).
>
English is not my first language but I chose error-out because WHEN
condition for COPY TO seems to me semantically incorrect

>
> AFAICS we could just get rid of the extra when_cluase variable and mess
> with the cstate->whenClause directly, depending on how (3) gets fixed.
>
I did it this way because CopyState structure memory allocate and
initialize in BeginCopyFrom but the analysis done before it

>
> 5) As I mentioned, the CREATE TRIGGER already has WHEN clause, but it
> requires it to be 'WHEN (expr)'. I suggest we do the same thing here,
> requiring the parentheses.
>
>
> 6) The skip logic in CopyFrom() seems to be slightly wrong. It does
> work, but the next_record label is defined after CHECK_FOR_INTERRUPTS()
> so a COPY will not respond to Ctrl-C unless it finds a row matching the
> WHEN condition. If you have a highly selective condition, that's a bit
> inconvenient.
>
> It also skips
>
> MemoryContextSwitchTo(GetPerTupleMemoryContext(estate));
>
> so I wonder what the heap_form_tuple() right after the next_record label
> will use for tuples right after a skipped one. I'd bet it'll use the
> oldcontext (essentially the long-lived context), essentially making it
> a memory leak.
>
> So I suggest to get rid of the next_record label, and use 'continue'
> instead of the 'goto next_record'.
>
> fixed

> regards
>
> --
> Tomas Vondra  http://www.2ndQuadrant.com
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>
diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
index 13a8b68d95..f9350afcdb 100644
--- a/doc/src/sgml/ref/copy.sgml
+++ b/doc/src/sgml/ref/copy.sgml
@@ -25,6 +25,7 @@ PostgreSQL documentation
 COPY table_name [ ( column_name [, ...] ) ]
 FROM { 'filename' | PROGRAM 'command' | STDIN }
 [ [ WITH ] ( option [, ...] ) ]
+[ WHEN ( condition ) ]
 
 COPY { table_name [ ( column_name [, ...] ) ] | ( query ) }
 TO { 'filename' | PROGRAM 'command' | STDOUT }
@@ -364,6 +365,30 @@ COPY { table_name [ ( 

 
+   
+WHEN
+
+   
+The optional WHEN clause has the general form
+
+WHEN condition
+
+where condition is
+any expression that evaluates to a result of type
+boolean.  Any row that does not satisfy this
+condition will not be inserted to the table.  A row satisfies the
+condition if it returns true when the actual row values are
+substituted for any variable references.
+   
+
+   
+Currently, WHEN expressions cannot contain
+subqueries.
+   
+
+
+   
+
   
  
 
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 9bc67ce60f..e70c315561 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -38,7 +38,10 @@
 #include "miscadmin.h"
 #include "optimizer/clauses.h"
 #include "optimizer/planner.h"
+#include "optimizer/prep.h"
 #include "nodes/makefuncs.h"
+#include "parser/parse_coerce.h"
+#include "parser/parse_expr.h"
 #include "parser/parse_relation.h"
 #include "port/pg_bswap.h"
 #include "rewrite/rewriteHandler.h"
@@ -147,6 +150,7 @@ typedef struct CopyStateData
 	bool		convert_selectively;	/* do selective binary conversion? */
 	List	   *convert_select; /* list of column names (can be NIL) */
 	bool	   *convert_select_flags;	/* per-column CSV/TEXT CS flags */
+	Node	   *whenClause;	/* WHEN 

RE: COPY FROM WHEN condition

[myungkyu.lim]

Hello,

Basically, this patch worked very well in my tests.

> 3) For COPY TO, the WHEN clause is accepted but ignored, leading to confusing 
> cases like this:

I found same issue.

postgres=# copy t1 to '/home/lmk/t1.data' when c1 < 1;

In the 'COPY TO' statement, 'WHEN clause' does not do any extra work.
(include error or hint)

> 4) There are some minor code style issues in copy.c - the variable is 
> misnamed as when_cluase, there are no spaces after 'if' etc. See the attached 
> patch fixing this.

It is recommended to use 'pg tool' when you finish development.

src/tools/pgindent/pgindent

pgindent is used to fix the source code style to conform to pg standards.

Best regards,
Myungkyu, Lim

Re: COPY FROM WHEN condition

[Tomas Vondra]

Hi,

I've taken a quick look at this on the way back from pgconf.eu, and it
seems like a nice COPY improvement in a fairly good shape.

Firstly, I think it's a valuable because it allows efficiently importing
a subset of data. Currently, we either have to create an intermediate
table, copy all the data into that, do the filtering, and then insert
the subset into the actual target table. Another common approach that I
see in practice is using file_fdw to do this, but it's not particularly
straight-forward (having to create the FDW servers etc. first) not
efficient (particularly for large amounts of data). This feature allows
skipping this extra step (at least in simpler ETL cases).

So, I like the idea and I think it makes sense.


A couple of comments regarding the code/docs:


1) I think this deserves at least some regression tests. Plenty of tests
already use COPY, but there's no coverage for the new piece. So let's
add a new test suite, or maybe add a couple of tests into copy2.sql.


2) In copy.sqml, the new item is defined like this

WHEN Clause

I suggest we use just WHEN, that's what
the other items do (see ENCODING).

The other thing is that this does not say what expressions are allowed
in the WHEN clause. It seems pretty close to WHEN clause for triggers,
which e.g. mentions that subselects are not allowed. I'm pretty sure
that's true here too, because it fails like this (118 = T_SubLink):

test=# copy t(a,b,c) from '/tmp/t.data' when ((select 1) < 10);
ERROR:  unrecognized node type: 118

So, the patch needs to detect this, produce a reasonable error message
and document the limitations in copy.sqml, just like we do for CREATE
TRIGGER.


3) For COPY TO, the WHEN clause is accepted but ignored, leading to
confusing cases like this:

test=# copy t(a,b,c) to '/tmp/t.data' when ((select 100) < 10);
COPY 151690

So, it contains subselect, but unlike COPY FROM it does not fail
(because we never execute it). The fun part is that the expression is
logically false, so a user might expect it to filter rows, yet we copy
everything.

IMHO we need to either error-out in these cases, complaining about WHEN
not being supported for COPY TO, or make it work (effectively treating
it as a simpler alternative to COPY (subselect) TO).


4) There are some minor code style issues in copy.c - the variable is
misnamed as when_cluase, there are no spaces after 'if' etc. See the
attached patch fixing this.

AFAICS we could just get rid of the extra when_cluase variable and mess
with the cstate->whenClause directly, depending on how (3) gets fixed.


5) As I mentioned, the CREATE TRIGGER already has WHEN clause, but it
requires it to be 'WHEN (expr)'. I suggest we do the same thing here,
requiring the parentheses.


6) The skip logic in CopyFrom() seems to be slightly wrong. It does
work, but the next_record label is defined after CHECK_FOR_INTERRUPTS()
so a COPY will not respond to Ctrl-C unless it finds a row matching the
WHEN condition. If you have a highly selective condition, that's a bit
inconvenient.

It also skips

MemoryContextSwitchTo(GetPerTupleMemoryContext(estate));

so I wonder what the heap_form_tuple() right after the next_record label
will use for tuples right after a skipped one. I'd bet it'll use the
oldcontext (essentially the long-lived context), essentially making it
a memory leak.

So I suggest to get rid of the next_record label, and use 'continue'
instead of the 'goto next_record'.


regards

-- 
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From fcf4b43dae7062f4786536fb262a665ad0b05b26 Mon Sep 17 00:00:00 2001
From: Tomas Vondra 
Date: Sun, 28 Oct 2018 16:09:37 +0100
Subject: [PATCH 2/2] review

---
 doc/src/sgml/ref/copy.sgml |  2 +-
 src/backend/commands/copy.c| 22 +++---
 src/backend/parser/gram.y  |  2 +-
 src/include/nodes/parsenodes.h |  2 +-
 4 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
index 8088e779b6..17bedf95cc 100644
--- a/doc/src/sgml/ref/copy.sgml
+++ b/doc/src/sgml/ref/copy.sgml
@@ -366,7 +366,7 @@ COPY { table_name [ ( 
 

-WHEN Clause
+WHEN
 

 The optional WHEN clause has the general form
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 192a1c576b..8e87605135 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -803,7 +803,7 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt,
 	Relation	rel;
 	Oid			relid;
 	RawStmt*query = NULL;
-	Node*when_cluase= NULL;
+	Node	   *whenClause = NULL;
 
 	/*
 	 * Disallow COPY to/from file or program except to users with the
@@ -857,19 +857,19 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt,
 			NULL, false, false);
 		rte->requiredPerms = (is_from ? ACL_INSERT : ACL_SELECT);
 
-		if(stmt->whenClause)
+		if (stmt->whenClause)
 		{
 			/* add rte 

Re: COPY FROM WHEN condition

[Surafel Temesgen]

On Fri, Oct 26, 2018 at 9:09 AM 임명규  wrote:

> Hello,
>
> I want test this patch, but can't apply it.
>
> (master)$ git apply copy_from_when_con_v1.patch
>
i use patch and it work

try (master)$ patch -p1 < copy_from_when_con_v1.patch

regards

Surafel

RE: COPY FROM WHEN condition

[임명규]

Hello,

I want test this patch, but can't apply it.

(master)$ git apply copy_from_when_con_v1.patch
error: patch failed: src/backend/commands/copy.c:849
error: src/backend/commands/copy.c: patch does not apply

fix or another way, let me know.

Best regards,
Myungkyu, Lim

Re: COPY FROM WHEN condition

[David Fetter]

On Thu, Oct 11, 2018 at 05:12:48AM -0400, Corey Huinker wrote:
> On Thu, Oct 11, 2018 at 5:04 AM Surafel Temesgen 
> wrote:
> 
> >
> >
> > On Thu, Oct 11, 2018 at 12:00 PM Christoph Moench-Tegeder <
> > c...@burggraben.net> wrote:
> >
> >> You can:
> >>   COPY ( query ) TO 'filename';
> >>
> > it is for COPY FROM
> >
> > regards
> > Surafel
> >
> 
> It didn't get far, but you may want to take a look at a rejected patch for
> copy_srf() (set returning function)
> https://www.postgresql.org/message-id/CADkLM%3DdoeiWQX4AGtDNG4PsWfSXz3ai7kY%3DPZm3sUhsUeev9Bg%40mail.gmail.com
> https://commitfest.postgresql.org/12/869/
> 
> Having a set returning function gives you the full expressiveness of SQL,
> at the cost of an extra materialization step.

I wonder whether something JIT-like could elide this. A very
interesting subset of such WHEN clauses could be pretty
straight-forward to implement in a pretty efficient way.

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

Re: COPY FROM WHEN condition

[Corey Huinker]

On Thu, Oct 11, 2018 at 5:04 AM Surafel Temesgen 
wrote:

>
>
> On Thu, Oct 11, 2018 at 12:00 PM Christoph Moench-Tegeder <
> c...@burggraben.net> wrote:
>
>> You can:
>>   COPY ( query ) TO 'filename';
>>
> it is for COPY FROM
>
> regards
> Surafel
>

It didn't get far, but you may want to take a look at a rejected patch for
copy_srf() (set returning function)
https://www.postgresql.org/message-id/CADkLM%3DdoeiWQX4AGtDNG4PsWfSXz3ai7kY%3DPZm3sUhsUeev9Bg%40mail.gmail.com
https://commitfest.postgresql.org/12/869/

Having a set returning function gives you the full expressiveness of SQL,
at the cost of an extra materialization step.

Re: COPY FROM WHEN condition

[Surafel Temesgen]

On Thu, Oct 11, 2018 at 12:00 PM Christoph Moench-Tegeder <
c...@burggraben.net> wrote:

> You can:
>   COPY ( query ) TO 'filename';
>
it is for COPY FROM

regards
Surafel

Re: COPY FROM WHEN condition

[Christoph Moench-Tegeder]

## Surafel Temesgen (surafel3...@gmail.com):

> Currently we can not moves data from a file to a table based on some
> condition on a certain column

You can:
  COPY ( query ) TO 'filename';

There's even an example in the documentation:
https://www.postgresql.org/docs/10/static/sql-copy.html
"To copy into a file just the countries whose names start with 'A':
 COPY (SELECT * FROM country WHERE country_name LIKE 'A%') TO 
'/usr1/proj/bray/sql/a_list_countries.copy';"

Regards,
Christoph

-- 
Spare Space.