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

Hi,

In <CAD21AoBpWFU4k-_bwrTq0AkFSAdwQqhAsSW188STmu9HxLJ0nQ(at)mail(dot)gmail(dot)com>
"Re: Make COPY format extendable: Extract COPY TO format implementations" on Fri, 31 Jan 2025 14:25:34 -0800,
Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:

> I think that CopyToState and CopyFromState are not APIs but the
> execution states. I'm not against exposing CopyToState and
> CopyFromState. What I'd like to avoid is that we end up adding
> everything (including new fields we add in the future) related to copy
> operation to copyapi.h, leading to include copyapi.h into files that
> are not related to custom format api. fdwapi.h and tsmapi.h as
> examples have only a struct having a bunch of callbacks but not the
> execution state data such as SampScanState are not defined there.

Thanks for sharing examples. But it seems that
fdwapi.h/tsmapi.h (ForeignScanState/SampleScanSate) are not
good examples. It seems that PostgreSQL uses
nodes/execnodes.h for all *ScanState. It seems that the
sparation is not related to *api.h usage.

> My understanding is that we don't strictly prohibit _internal.h from
> being included in out of core files. For example, file_fdw.c includes
> copyfrom_internal.h in order to access some fields of CopyFromState.
>
> If the name with _internal.h is the problem, we can rename them to
> copyfrom.h and copyto.h. It makes sense to me that the code that needs
> to access the internal of the copy execution state include _internal.h
> header, though.

Thanks for sharing the file_fdw.c example. I'm OK with
_internal.h suffix because PostgreSQL doesn't prohibit
_internal.h usage by extensions as you mentioned.

>> > While we get the format routines for custom formats in
>> > ProcessCopyOptionFormat(), we do that for built-in formats in
>> > BeginCopyTo(), which seems odd to me. I think we can have
>> > CopyToGetRoutine() responsible for getting CopyToRoutine for built-in
>> > formats as well as custom format. The same is true for
>> > CopyFromRoutine.
>>
>> I like the current design because we don't need to export
>> CopyToGetBuiltinRoutine() (we can use static for
>> CopyToGetBuiltinRoutine()) but I applied your
>> suggestion. Because it's not a strong opinion.
>
> I meant that ProcessCopyOptionFormat() doesn't not necessarily get the
> routine. An idea is that in ProcessCopyOptionFormat() we just get the
> OID of the handler function, and then set up the format routine in
> BeginCopyTo(). I've attached a patch for this idea (applied on top of
> 0009).

Oh, sorry. I misunderstood your suggestion. I understand
what you suggested by the patch. Thanks.

If we use the approach, we can't show error position when a
custom COPY format handler function returns invalid routine
because DefElem for the "format" option isn't available in
BeginCopyTo(). Is it acceptable? If it's acceptable, let's
use the approach.

> Also, please check some regression test failures on cfbot.

Oh, sorry. I forgot to follow function name change in
0009. I attach the v30 patch set that fixes it in 0009.

Thanks,
--
kou