Re: Make COPY format extendable: Extract COPY TO format implementations

Hi,

In <CAD21AoBW5dEv=Gd2iF_BYNZGEsF=3KTG7fpq=vP5qwpC1CAOeA(at)mail(dot)gmail(dot)com>
"Re: Make COPY format extendable: Extract COPY TO format implementations" on Mon, 25 Nov 2024 23:10:50 -0800,
Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:

>> Custom COPY format extensions need to use
>> CopyToStateData/CopyFromStateData. For example,
>> CopyToStateData::rel is used to retrieve table schema. If we
>> move CopyToStateData to copyto_internal.h not copyapi.h,
>> custom COPY format extensions need to include
>> copyto_internal.h. I feel that it's strange that extensions
>> need to use internal headers.
>>
>> What is your real concern? If you don't want to export
>> CopyToStateData/CopyFromStateData entirely, we can provide
>> accessors only for some members of them.
>
> I'm not against exposing CopyToStateData and CopyFromStateData. My
> concern is that if we move all copy-related structs to copyapi.h,
> other copy-related files would need to include copyapi.h even if the
> file is not related to copy format APIs. IMO copyapi.h should have
> only copy-format-API-related variables structs such as CopyFromRoutine
> and CopyToRoutine and functions that custom COPY format extension can
> utilize to access data source and destination, such as CopyGetData().
>
> When it comes to CopyToStateData and CopyFromStateData, I feel that
> they have mixed fields of common fields (e.g., rel, num_errors, and
> transition_capture) and format-specific fields (e.g., input_buf,
> line_buf, and eol_type). While it makes sense to me that custom copy
> format extensions can access the common fields, it seems odd to me
> that they can access text-and-csv-format-specific fields such as
> input_buf. We might want to sort out these fields but it could be a
> huge task.

I understand you concern.

How about using Copy{To,From}StateData::opaque to store
text-and-csv-format-specific data? I feel that this
refactoring doesn't block the 0001/0002 patches. Do you
think that this is a blocker of the 0001/0002 patches?

I think that this may block the 0004/0007 patches that
export Copy{To,From}StateData. But we can work on it after
we merge the 0004/0007 patches. Which is preferred?

> Also, I realized that CopyFromTextLikeOneRow() does input function
> calls and handle soft errors based on ON_ERROR and LOG_VERBOSITY
> options. I think these should be done in the core side.

How about extracting the following part in NextCopyFrom() as
a function and provide it for extensions?

----
Assert(cstate->opts.on_error != COPY_ON_ERROR_STOP);

cstate->num_errors++;

if (cstate->opts.log_verbosity == COPY_LOG_VERBOSITY_VERBOSE)
{
/*
* Since we emit line number and column info in the below
* notice message, we suppress error context information
* other than the relation name.
*/
Assert(!cstate->relname_only);
cstate->relname_only = true;

if (cstate->cur_attval)
{
char *attval;

attval = CopyLimitPrintoutLength(cstate->cur_attval);
ereport(NOTICE,
errmsg("skipping row due to data type incompatibility at line %llu for column \"%s\": \"%s\"",
(unsigned long long) cstate->cur_lineno,
cstate->cur_attname,
attval));
pfree(attval);
}
else
ereport(NOTICE,
errmsg("skipping row due to data type incompatibility at line %llu for column \"%s\": null input",
(unsigned long long) cstate->cur_lineno,
cstate->cur_attname));

/* reset relname_only */
cstate->relname_only = false;
}
----

See the attached v27 patch set for this idea.

0001-0008 are almost same as the v26 patch set.
("format" -> "FORMAT" in COPY test changes are included.)

0009 exports the above code as
CopyFromSkipErrorRow(). Extensions should call it when they
use errsave() for a soft error in CopyFromOneRow callback.

Thanks,
--
kou