| From: | Dilip Kumar <dilipbalaut(at)gmail(dot)com> |
|---|---|
| To: | Sami Imseih <samimseih(at)gmail(dot)com> |
| Cc: | Michael Paquier <michael(at)paquier(dot)xyz>, Nikolay Samokhvalov <nik(at)postgres(dot)ai>, pgsql-hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
| Subject: | Re: IO wait events for COPY FROM/TO PROGRAM or file |
| Date: | 2026-01-09 03:44:07 |
| Message-ID: | CAFiTN-tKDKY_fPy5iacPWQ7vnY5qWkvS2kCnieeE3Uy9Uj6JGQ@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
On Fri, Jan 9, 2026 at 8:52 AM Sami Imseih <samimseih(at)gmail(dot)com> wrote:
>
> Hi,
>
> Thanks for this patch. I have a few comments.
>
> > Hmm. Makes sense to me (perhaps you have played with an LLM to find
> > out fread() and fwrite() patterns that would need an event?), but I
> > don't think that the part about COPY TO is done right. The wait event
> > should be set during the fwrite() call only, similarly to the COPY
> > FROM case, by saving the result returned by fwrite() in a separate
> > variable.
>
> I see other cases such as inside copy_file() in which the read() is wrapped
> by wait_start and wait_end, but in the write() case, the wait_end() is after
> the error handling ( as the attached v1 ).
>
> ```
> pgstat_report_wait_start(WAIT_EVENT_COPY_FILE_READ);
> nbytes = read(srcfd, buffer, COPY_BUF_SIZE);
> pgstat_report_wait_end();
> .....
> ..........
> pgstat_report_wait_start(WAIT_EVENT_COPY_FILE_WRITE);
> if ((int) write(dstfd, buffer, nbytes) != nbytes)
> {
> /* if write didn't set errno, assume problem is no disk space */
> if (errno == 0)
> errno = ENOSPC;
> ereport(ERROR,
> (errcode_for_file_access(),
> errmsg("could not write to file \"%s\": %m", tofile)));
> }
> pgstat_report_wait_end();
> ```
> another example is write_relmap_file():
>
> ```
> /* Write new data to the file. */
> pgstat_report_wait_start(WAIT_EVENT_RELATION_MAP_WRITE);
> if (write(fd, newmap, sizeof(RelMapFile)) != sizeof(RelMapFile))
> {
> /* if write didn't set errno, assume problem is no disk space */
> if (errno == 0)
> errno = ENOSPC;
> ereport(ERROR,
> (errcode_for_file_access(),
> errmsg("could not write file \"%s\": %m",
> maptempfilename)));
> }
> pgstat_report_wait_end();
> ```
>
> I don't see any issue with this approach for fwrite().
>
> Another comment. Wouldn't be better to use "COPY FROM" and "COPY TO" in the
> names to make it more obvious they are related to the COPY command?
>
> so instead of:
>
> +COPY_DATA_READ "Waiting for a read from a file or program during COPY FROM."
> +COPY_DATA_WRITE "Waiting for a write to a file or program during COPY TO."
>
> this:
>
> COPY_FROM_READ: "Waiting to read data from a file or program during COPY FROM."
> COPY_TO_WRITE: "Waiting to write data to a file or program during COPY TO."
IMHO this suggestion makes sense to avoid confusion with similar wait
event names like COPY_FILE_READ etc. So using COPY_FROM and COPY_TO
is clearer for the purpose.
--
Regards,
Dilip Kumar
Google
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Xuneng Zhou | 2026-01-09 03:57:02 | Re: Optimize SnapBuildPurgeOlderTxn: use in-place compaction instead of temporary array |
| Previous Message | Sami Imseih | 2026-01-09 03:22:26 | Re: IO wait events for COPY FROM/TO PROGRAM or file |