On Fri, Jan 9, 2026 at 8:52 AM Sami Imseih <[email protected]> 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
