Re: PATCH: logical_work_mem and logical streaming of large in-progress transactions

On Fri, Apr 17, 2020 at 1:40 AM Kuntal Ghosh <kuntalghosh(dot)2007(at)gmail(dot)com> wrote:
>
> On Tue, Apr 14, 2020 at 3:41 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> >
>
> Few review comments from 0006-Add-support-for-streaming*.patch
>
> + subxacts[nsubxacts].offset = lseek(stream_fd, 0, SEEK_END);
> lseek can return (-)ve value in case of error, right?
>
> + /*
> + * We might need to create the tablespace's tempfile directory, if no
> + * one has yet done so.
> + *
> + * Don't check for error from mkdir; it could fail if the directory
> + * already exists (maybe someone else just did the same thing). If
> + * it doesn't work then we'll bomb out when opening the file
> + */
> + mkdir(tempdirpath, S_IRWXU);
> If that's the only reason, perhaps we can use something like following:
>
> if (mkdir(tempdirpath, S_IRWXU) < 0 && errno != EEXIST)
> throw error;

Done

>
> +
> + CloseTransientFile(stream_fd);
> Might failed to close the file. We should handle the case.

Changed

Still, one place is pending because I don't have the filename there to
report an error. One option is we can just give an error without the
filename. I will try to think about this part.

> Also, I think we need some implementations in dumpSubscription() to
> dump the (streaming = 'on') option.

Right, created another patch and attached.

I have also fixed a couple of bugs internally reported by my colleague
Neha Sharma.

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