Re: PATCH: logical_work_mem and logical streaming of largein-progress transactions - Mailing list pgsql-hackers
From | Amit Kapila |
---|---|
Subject | Re: PATCH: logical_work_mem and logical streaming of largein-progress transactions |
Date | |
Msg-id | CAA4eK1LFG3JouWFx6N-STMDPNOK=i7rX1NAnGDQ=WxjKMSttqg@mail.gmail.com Whole thread Raw |
In response to | Re: PATCH: logical_work_mem and logical streaming of largein-progress transactions (Dilip Kumar <dilipbalaut@gmail.com>) |
Responses |
Re: PATCH: logical_work_mem and logical streaming of largein-progress transactions
Re: PATCH: logical_work_mem and logical streaming of largein-progress transactions |
List | pgsql-hackers |
On Mon, Jun 15, 2020 at 9:12 AM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > On Fri, Jun 12, 2020 at 4:35 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > > > > > Basically, this part is still > > > I have to work upon, once we get the consensus then I can remove those > > > extra wait event from the patch. > > > > > > > Okay, feel free to send an updated patch with the above change. > > Sure, I will do that in the next patch set. > I have few more comments on the patch 0013-Change-buffile-interface-required-for-streaming-.patch: 1. - * temp_file_limit of the caller, are read-only and are automatically closed - * at the end of the transaction but are not deleted on close. + * temp_file_limit of the caller, are read-only if the flag is set and are + * automatically closed at the end of the transaction but are not deleted on + * close. */ File -PathNameOpenTemporaryFile(const char *path) +PathNameOpenTemporaryFile(const char *path, int mode) No need to say "are read-only if the flag is set". I don't see any flag passed to function so that part of the comment doesn't seem appropriate. 2. @@ -68,7 +68,8 @@ SharedFileSetInit(SharedFileSet *fileset, dsm_segment *seg) } /* Register our cleanup callback. */ - on_dsm_detach(seg, SharedFileSetOnDetach, PointerGetDatum(fileset)); + if (seg) + on_dsm_detach(seg, SharedFileSetOnDetach, PointerGetDatum(fileset)); } Add comments atop function to explain when we don't want to register the dsm detach stuff? 3. + */ + newFile = file->numFiles - 1; + newOffset = FileSize(file->files[file->numFiles - 1]); break; FileSize can return negative lengths to indicate failure which we should handle. See other places in the code where FileSize is used? But I have another question here which is why we need to implement SEEK_END? How other usages of BufFile interface takes care of this? I see an API BufFileTell which can give the current read/write location in the file, isn't that sufficient for your usage? Also, how before BufFile usage is this thing handled in the patch? 4. + /* Loop over all the files upto the fileno which we want to truncate. */ + for (i = file->numFiles - 1; i >= fileno; i--) "the files", extra space in the above part of the comment. 5. + /* + * Except the fileno, we can directly delete other files. Before 'we', there is extra space. 6. + else + { + FileTruncate(file->files[i], offset, WAIT_EVENT_BUFFILE_READ); + newOffset = offset; + } The wait event passed here doesn't seem to be appropriate. You might want to introduce a new wait event WAIT_EVENT_BUFFILE_TRUNCATE. Also, the error handling for FileTruncate is missing. 7. + if ((i != fileno || offset == 0) && fileno != 0) + { + SharedSegmentName(segment_name, file->name, i); + SharedFileSetDelete(file->fileset, segment_name, true); + newFile--; + newOffset = MAX_PHYSICAL_FILESIZE; + } Similar to the previous comment, I think we should handle the failure of SharedFileSetDelete. 8. I think the comments related to BufFile shared API usage need to be expanded in the code to explain the new usage. For ex., see the below comments atop buffile.c * BufFile supports temporary files that can be made read-only and shared with * other backends, as infrastructure for parallel execution. Such files need * to be created as a member of a SharedFileSet that all participants are * attached to. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
pgsql-hackers by date: