Re: Make COPY format extendable: Extract COPY TO format implementations - Mailing list pgsql-hackers

On Mon, Sep 8, 2025 at 7:50 PM Sutou Kouhei <kou@clear-code.com> wrote:
>
> Hi,
>
> In <CAD21AoCCjKA77xkUxx59qJ8an_G_58Mry_EtCEcFgd=g9N2xew@mail.gmail.com>
>   "Re: Make COPY format extendable: Extract COPY TO format implementations" on Mon, 8 Sep 2025 14:08:16 -0700,
>   Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> >> > The fields in 1 are mostly static fields, and the fields in 2 and 3
> >> > are likely to be accessed in hot functions during COPY FROM. Would it
> >> > be a good idea to restructure these fields so that we can hide the
> >> > fields in 1 from callback functions and having the fields in 3 in a
> >> > separate format-specific struct that can be accessed via an opaque
> >> > pointer? But could the latter change potentially cause performance
> >> > overheads?
> >>
> >> Yes. It changes memory layout (1 continuous memory chunk ->
> >> 2 separated memory chunks) and introduces indirect member
> >> accesses (x->y -> x->z->y).
> >
> > I think fields accessed by the hot functions are limited. If we
> > assemble fields required by hot functions into one struct and pass it
> > to them can we deal with the latter? For example, we assemble the
> > fields in 3 I mentioned above (i.e., built-in format specific fields)
> > into say CopyFromStateBuiltin and pass it to CopyReadLine() function
> > and the following functions, instead of CopyFromState. Since there are
> > some places where we need to access to CopyFromState (e.g.,
> > CopyGetData()), CopyFromStateBuiltin needs to have a pointer to
> > CopyFromState as well.
>
> It can change indirect member accesses (built-in format
> specific members can be direct access but other members in
> CopyFromState are indirect access) but it doesn't change 2
> separated memory chunks.

Right. IIUC the latter point is related to cache locality. But I'm not
sure how much the latter point affects the performance as currently we
don't declare fields to CopyFromState while carefully considering the
cache locality even today. Separating a single memory into multiple
chunks could even have a positive effect on it.

>
> If this approach has performance impact and it's caused by
> indirect member accesses for built-in format specific
> members, your suggestion will work. If performance impact is
> caused by another reason, your suggestion may not work.
>
> Anyway, we need to measure performance to proceed with this
> approach. If we can confirm that this approach doesn't have
> any performance impact, we can use the original your idea.
>
> Do you have any idea how to measure performance of this
> approach?

I think we can start with measuring the entire COPY execution time
with several scenarios as we did previously. For example, reading a
huge file with a single column value and with many columns etc.

>
> We did it when we introduce Copy{To,From}Routine. But it was
> difficult to evaluate the results:
>
> * I don't have machines for stable benchmark results
>   * We may not be able to use them for the final decision
> * Most results showed performance improvement but
>   there was a result showed mysterious result[1]

Perhaps measuring cache-misses help to see how changes could affect
the performance?

>
> [1] https://www.postgresql.org/message-id/flat/CAEG8a3LUBcvjwqgt6AijJmg67YN_b_NZ4Kzoxc_dH4rpAq0pKg%40mail.gmail.com
>
> >> Drawback: This approach always allocates
> >> CopyToStateInternalData not CopyToStateData. So we need to
> >> allocate needless memories for extensions. But this will
> >> prevent performance regression of built-in formats. Is it
> >> acceptable?
> >
> > While this approach could make sense to avoid potential performance
> > overheads for built-in format, I find it's somewhat odd that
> > extensions cannot allocate memory for its working state having
> > CopyToStateData as the base type.
>
> Is it important? We'll provide a opaque member for
> extensions. Extensions should use the opaque member instead
> of extending Copy{From,To}StateData.

I think yes, because it could be a blocker for future improvements
that might require a large field to CopyFrom/ToStateData.

> I don't object your approach but we need a good way to
> measure performance. If we use this approach, we can omit it
> for now and we can revisit your approach later without
> breaking compatibility. How about using this approach if we
> can't find a good way to measure performance?

I think it would be better to hear more opinions about this idea and
then make a decision, rather than basing our decision on whether or
not we can measure its performance, so we can be more confident in the
idea we have chosen. While this idea has the above downside, it could
make sense because we always allocate the entire CopyFrom/ToStateData
even today in spite of some fields being not used at all in binary
format and it requires less implementation costs to hide the
for-core-only fields. On the other hand, another possible idea is that
we have three different structs for categories 1 (core-only), 2 (core
and extensions), and 3 (extension-only), and expose only 2 that has a
void pointer to 3. The core can allocate the memory for 1 that embeds
2 at the beginning of the fields. While this design looks cleaner and
we can minimize overheads due to indirect references, it would require
more implementation costs. Which method we choose, I think we need
performance measurements in several scenarios to check if performance
regressions don't happen unexpectedly.


Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com