On Tue, Jan 30, 2024 at 05:15:11PM +0900, Sutou Kouhei wrote:
> We use defel->location for an error message. (We don't need
> to set location for the default "text" DefElem.)
Yeah, but you should not need to have this error in the paths that set
the callback routines in opts_out if the same validation happens a few
lines before, in copy.c.
> Yes, sure.
> (We want to focus on the performance gains in the first
> patch set and then focus on extensibility again, right?)
Yep, exactly, the numbers are too good to just ignore. I don't want
to hijack the thread, but I am really worried about the complexities
this thread is getting into because we are trying to shape the
callbacks in the most generic way possible based on *two* use cases.
This is going to be a never-ending discussion. I'd rather get some
simple basics, and then we can discuss if tweaking the callbacks is
really necessary or not. Even after introducing the pg_proc lookups
to get custom callbacks.
> For the purpose, I think that the v7 patch set is more
> suitable than the v9 patch set. The v7 patch set doesn't
> include Init() callbacks, custom options validation support
> or extra Copy{In,Out}Response support. But the v7 patch set
> misses the removal of the "if" checks in
> NextCopyFromRawFields() that exists in the v9 patch set. I'm
> not sure how much performance will improve by this but it
> may be worth a try.
Yeah.. The custom options don't seem like an absolute strong
requirement for the first shot with the callbacks or even the
possibility to retrieve the callbacks from a function call. I mean,
you could provide some control with SET commands and a few GUCs, at
least, even if that would be strange. Manipulations with a list of
DefElems is the intuitive way to have custom options at query level,
but we also have to guess the set of callbacks from this list of
DefElems coming from the query. You see my point, I am not sure
if it would be the best thing to process twice the options, especially
when it comes to decide if a DefElem should be valid or not depending
on the callbacks used. Or we could use a kind of "special" DefElem
where we could store a set of key:value fed to a callback :)
> Can I prepare the v10 patch set as "the v7 patch set" + "the
> removal of the "if" checks in NextCopyFromRawFields()"?
> (+ reverting opts_out->{csv_mode,binary} changes in
> ProcessCopyOptions().)
Yep, if I got it that would make sense to me. If you can do that,
that would help quite a bit. :)
--
Michael