Re: Adding Support for Copy callback functionality on COPY TO api

From: Soumyadeep Chakraborty <soumyadeep2007(at)gmail(dot)com>
To: "Sanaba, Bilva" <bilvas(at)amazon(dot)com>
Cc: "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Adding Support for Copy callback functionality on COPY TO api
Date: 2020-09-14 23:28:12
Message-ID: CAE-ML+_ebFzUnsgj-4MLshLGFfT05Lo15KK6rpcUt8-ZZKL1XQ@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi Bilva,

Thank you for registering this patch!

I had a few suggestions:

1. Please run pg_indent[1] on your code. Make sure you add
copy_data_destination_cb to src/tools/pgindent/typedefs.list. Please
run pg_indent on only the files you changed (it will take files as
command line args)

2. For features such as this, it is often helpful to find a use case
within backend/utility/extension code that demonstrate thes callback and
to include the code to exercise it with the patch. Refer how
copy_read_data() is used as copy_data_source_cb, to copy the data from
the query results from the WAL receiver (Refer: copy_table()). Finding
a similar use case in the source tree will make a stronger case
for this patch.

3. Wouldn't we want to return the number of bytes written from
copy_data_destination_cb? (Similar to copy_data_source_cb) We should
also think about how to represent failure. Looking at CopySendEndOfRow(),
we should error out like we do for the other copy_dests after checking the
return value for the callback invocation.

4.
> bool pipe = (cstate->filename == NULL) && (cstate->data_destination_cb == NULL);

I think a similar change should also be applied to BeginCopyFrom() and
CopyFrom(). Or even better, such code could be refactored to have a
separate destination type COPY_PIPE. This of course, will be a separate
patch. I think the above line is okay for this patch.

Regards,
Soumyadeep

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2020-09-15 00:08:58 Re: pg_restore causing deadlocks on partitioned tables
Previous Message David Rowley 2020-09-14 23:17:24 Re: Use incremental sort paths for window functions