Re: PATCH: logical_work_mem and logical streaming of largein-progress transactions - Mailing list pgsql-hackers

On Mon, Jun 15, 2020 at 6:30 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> 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.

Done

> 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?

Done,  I am planning to work on more cleaner function for on_proc_exit
as we discussed offlist.  I will work on this in the next version.

> 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.

Done

  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?

So far we never supported to open the file in write mode,  only we
create in write mode.  So if we have created the file and its open we
can always use BufFileTell, which will tell the current end location
of the file.  But, once we close and open again it always set to read
from the start of the file as per the current use case.  We need a way
to jump to the end of the last file for appending it.

> 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.

Fixed

> 5.
> + /*
> + * Except the fileno,  we can directly delete other files.
>
> Before 'we', there is extra space.

Done.

> 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.

Done

> 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.

Other fixes (offlist raised by my colleague Neha Sharma)
1. In BufFileTruncateShared, the files were not closed before
deleting.  (in 0013)
2. In apply_handle_stream_commit, the file name in debug message was
printed before populating the name (0014)
3. On concurrent abort we are truncating all the changes including
some incomplete changes,  so later when we get the complete changes we
don't have the previous changes,  e.g, if we had specinsert in the
last stream and due to concurrent abort detection if we delete that
changes later we will get spec_confirm without spec insert.  We could
have simply avoided deleting all the changes, but I think the better
fix is once we detect the concurrent abort for any transaction, then
why do we need to collect the changes for that, we can simply avoid
that.  So I have put that fix. (0006)

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com