Re: Speed up COPY FROM text/CSV parsing using SIMD - Mailing list pgsql-hackers

On Wed, Mar 04, 2026 at 06:15:53PM +0300, Nazir Bilal Yavuz wrote:
> +#ifndef USE_NO_SIMD
> +static bool CopyReadLineTextSIMDHelper(CopyFromState cstate, bool is_csv,
> +                                                                        bool *temp_hit_eof, int *temp_input_buf_ptr);
> +#endif

Should we inline this, too?

> +                             /*
> +                              * Do not disable SIMD when we hit EOL or EOF characters. In
> +                              * practice, it does not matter for EOF because parsing ends
> +                              * there, but we keep the behavior consistent.
> +                              */
> +                             if (!(simd_hit_eof || simd_hit_eol))
> +                                     cstate->simd_enabled = false;

nitpick: I would personally avoid disabling it for EOF.  It probably
doesn't amount to much, but I don't see any point in the extra
complexity/work solely for consistency.

> +                             /*
> +                              * We encountered a EOL or EOF on the first vector. This means
> +                              * lines are not long enough to skip fully sized vector. If
> +                              * this happens two times consecutively, then disable the
> +                              * SIMD.
> +                              */
> +                             if (first_vector)
> +                             {
> +                                     if (cstate->simd_failed_first_vector)
> +                                             cstate->simd_enabled = false;
> +
> +                                     cstate->simd_failed_first_vector = true;
> +                             }

The first time I saw this, my mind immediately went to the extreme case
where this likely regresses: alternating long and short lines.  We might
just want to disable it the first time we see a short line, like we do for
special characters.  This is another thing that we can improve
independently later on.

> +     /* First try to run SIMD, then continue with the scalar path */
> +     if (cstate->simd_enabled)
> +     {
> +             int                     temp_input_buf_ptr = input_buf_ptr;
> +             bool            temp_hit_eof = false;
> +
> +             result = CopyReadLineTextSIMDHelper(cstate, is_csv, &temp_hit_eof,
> +                                                                                     &temp_input_buf_ptr);
> +             input_buf_ptr = temp_input_buf_ptr;
> +             hit_eof = temp_hit_eof;

Given CopyReadLineTextSIMDHelper() doesn't have too much duplicated code,
moving the SIMD stuff to its own function is nice.  The temp variables seem
a bit too magical to me, though.  If those really make a difference, IMHO
there ought to be a big comment explaining why.

--
nathan