On Tue, Jun 24, 2025 at 4:10 PM Sutou Kouhei <k...@clear-code.com> wrote:
>
> Hi,
>
> In <CAD21AoC8-d=GF-hOvGqUyq2xFg=qgpyfciwjbcp4wcn0uid...@mail.gmail.com>
> "Re: Make COPY format extendable: Extract COPY TO format implementations"
> on Tue, 24 Jun 2025 15:24:23 +0900,
> Masahiko Sawada <sawada.m...@gmail.com> wrote:
>
> >> It's natural to add more related APIs with this
> >> approach. The single registration API provides one feature
> >> by one operation. If we use the RegisterCopyRoutine() for
> >> FROM and TO formats API, it's not natural that we add more
> >> related APIs. In this case, some APIs may provide multiple
> >> features by one operation and other APIs may provide single
> >> feature by one operation. Developers may be confused with
> >> the API. For example, developers may think "what does mean
> >> NULL here?" or "can we use NULL here?" for
> >> "RegisterCopyRoutine("new-format", NewFormatFromRoutine,
> >> NULL)".
> >
> > We can document it in the comment for the registration function.
>
> I think that API that can be understandable without the
> additional note is better API than API that needs some
> notes.
I don't see much difference in this case.
>
> Why do you suggest the RegisterCopyRoutine("new-format",
> NewFormatFromRoutine, NewFormatToRoutine) API? You want to
> remove the duplicated codes in
> RegisterCopy{From,To}Routine(), right?
No. I think that if extensions are likely to support both
CopyToRoutine and CopyFromRoutine in most cases, it would be simpler
to register the custom format using a single API. Registering
CopyToRoutine and CopyFromRoutine separately seems redundant to me.
>
> BTW, what do you think about my answer (one feature by one
> operation API is more extendable API) for your question
> (extendability and API compatibility)?
Could you provide some examples? It seems to me that even if we
provide the single API for the registration we can provide other APIs
differently. For example, if we want to provide an API to register a
custom option, we can provide RegisterCopyToOption() and
RegisterCopyFromOption().
>
> > Suppose that extension-A implements only CopyToRoutine for the
> > custom-format-X with the format name 'myformat' and extension-B
> > implements only CopyFromRoutine for the custom-format-Y with the same
> > name, if users load both extension-A and extension-B, it seems to me
> > that extension-A registers the custom-format-X format as 'myformat'
> > only with CopyToRoutine, and extension-B overwrites the 'myformat'
> > registration by adding custom-format-Y's CopyFromRoutine. However, if
> > users register extension-C that implements both routines with the
> > format name 'myformat', they can register neither extension-A nor
> > extension-B, which seems to me that we don't allow overwriting the
> > registration in this case.
>
> Do you assume that users use extension-A, extension-B and
> extension-C without reading their documentation? If users
> read their documentation before users use them, users can
> know all of them use the same format name 'myformat' and
> which extension provides Copy{From,To}Routine.
>
> In this case, these users (who don't read documentation)
> will be confused with the RegisterCopyRoutine("new-format",
> NewFormatFromRoutine, NewFormatToRoutine) API too. Do we
> really need to care about this case?
My point is about the consistency of registration behavior. I think
that we should raise an error if the custom format name that an
extension tries to register already exists. Therefore I'm not sure why
installing extension-A+B is okay but installing extension-C+A or
extension-C+B is not okay? We can think that's an extension-A's choice
not to implement CopyFromRoutine for the 'myformat' format so
extension-B should not change it.
>
> > I think the core issue appears to be the internal management of custom
> > format entries but the current patch does enable registration
> > overwriting in the former case (extension-A and extension-B case).
>
> This is the same behavior as existing custom EXPLAIN option
> implementation. Should we use different behavior here?
I think that unlike custom EXPLAIN options, it's better to raise an
error or a warning if the custom format name (or combination of format
name and COPY direction) that an extension tries to register already
exists.
> >> >> FYI: RegisterCopy{From,To}Routine() uses the same logic
> >> >> as RegisterExtensionExplainOption().
> >> >
> >> > I'm concerned that the patch has duplicated logics for the
> >> > registration of COPY FROM and COPY TO.
> >>
> >> We can implement a convenient routine that can be used for
> >> RegisterExtensionExplainOption() and
> >> RegisterCopy{From,To}Routine() if it's needed.
> >
> > I meant there are duplicated codes in COPY FROM and COPY TO. For
> > instance, RegisterCopyFromRoutine() and RegisterCopyToRoutine() have
> > the same logic.
>
> Yes, I understand it. I wanted to say that we can remove the
> duplicated codes by introducing a RegisterSomething()
> function that can be used by
> RegisterExtensionExplainOption() and
> RegisterCopy{From,To}Routine():
>
> void
> RegisterSomething(...)
> {
> /* Common codes in RegisterExtensionExplainOption() and
> RegisterCopy{From,To}Routine()
> ...
> */
> }
>
> void
> RegisterExtensionExplainOption(...)
> {
> RegisterSomething(...);
> }
>
> void
> RegisterCopyFromRoutine(...)
> {
> RegisterSomething(...);
> }
>
> void
> RegisterCopyToRoutine(...)
> {
> RegisterSomething(...);
> }
>
> You think that this approach can't remove the duplicated
> codes, right?
Well, no, I just meant we don't need to do that. Custom EXPLAIN option
and custom COPY format are different features and have different
requirements. I think while we don't need to remove duplicates between
them at least at this stage we need to remove the duplicate between
COPY TO registration code and COPY TO's one.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
