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

On Sat, Aug 15, 2020 at 3:32 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> On Thu, Aug 13, 2020 at 6:47 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Thu, Aug 13, 2020 at 12:08 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > >
> > > On Fri, Aug 7, 2020 at 2:04 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> > > >
> > > > On Thu, Aug 6, 2020 at 2:43 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > > > >
> > > ..
> > > > > This patch's functionality can be independently verified by SQL APIs
> > > >
> > > > Your changes look fine to me.
> > > >
> > >
> > > I have pushed that patch last week and attached are the remaining
> > > patches. I have made a few changes in the next patch
> > > 0001-Extend-the-BufFile-interface.patch and have some comments on it
> > > which are as below:
> > >
> >
> > Few more comments on the latest patches:
> > v48-0002-Add-support-for-streaming-to-built-in-replicatio
> > 1. It appears to me that we don't remove the temporary folders created
> > by the apply worker. So, we have folders like
> > pgsql_tmp15324.0.sharedfileset in base/pgsql_tmp directory even when
> > the apply worker exits. I think we can remove these by calling
> > PathNameDeleteTemporaryDir in SharedFileSetUnregister while removing
> > the fileset from registered filesetlist.
>
> I think we need to call SharedFileSetDeleteAll(input_fileset), from
> SharedFileSetUnregister, so that all the directories created for this
> fileset are removed
>
> > 2.
> > +typedef struct SubXactInfo
> > +{
> > + TransactionId xid; /* XID of the subxact */
> > + int fileno; /* file number in the buffile */
> > + off_t offset; /* offset in the file */
> > +} SubXactInfo;
> > +
> > +static uint32 nsubxacts = 0;
> > +static uint32 nsubxacts_max = 0;
> > +static SubXactInfo *subxacts = NULL;
> > +static TransactionId subxact_last = InvalidTransactionId;
> >
> > Will it be better if we move all the subxact related variables (like
> > nsubxacts, nsubxacts_max and subxact_last) inside SubXactInfo struct
> > as all the information anyway is related to sub-transactions?
>
> I have moved them all to a structure.
>
> > 3.
> > + /*
> > + * If there is no subtransaction then nothing to do, but if already have
> > + * subxact file then delete that.
> > + */
> >
> > extra space before 'but' in the above sentence is not required.
>
> Fixed
>
> > v48-0001-Extend-the-BufFile-interface
> > 4.
> > - * SharedFileSets can also be used by backends when the temporary files need
> > - * to be opened/closed multiple times and the underlying files need to survive
> > + * SharedFileSets can be used by backends when the temporary files need to be
> > + * opened/closed multiple times and the underlying files need to survive
> > * across transactions.
> > *
> >
> > No need of 'also' in the above sentence.
>
> Fixed
>

In last patch v49-0001, there is one issue, Basically, I have called
BufFileFlush in all the cases. But, ideally, we can not call this if
the underlying files are deleted/truncated because those files/blocks
might not exist now. So I think if the truncate position is within
the same buffer we just need to adjust the buffer, otherwise we just
need to set the currFile and currOffset to the absolute number and set
the pos and nbytes 0. Attached patch fixes this issue.

+ errmsg("could not truncate file \"%s\": %m",
+ FilePathName(file->files[i]))));
+ curOffset = offset;
+ }
+ }
+
+ /* Otherwise, must reposition buffer, so flush any dirty data */
+ BufFileFlush(file);
+

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