On Tue, Aug 31, 2021 at 3:20 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Aug 27, 2021 at 12:04 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
>
> Few comments on v6-0002*
> =========================
> 1.
> -BufFileDeleteFileSet(FileSet *fileset, const char *name)
> +BufFileDeleteFileSet(FileSet *fileset, const char *name, bool missing_ok)
> {
> char segment_name[MAXPGPATH];
> int segment = 0;
> @@ -358,7 +369,7 @@ BufFileDeleteFileSet(FileSet *fileset, const char *name)
> for (;;)
> {
> FileSetSegmentName(segment_name, name, segment);
> - if (!FileSetDelete(fileset, segment_name, true))
> + if (!FileSetDelete(fileset, segment_name, !missing_ok))
>
> I don't think the usage of missing_ok is correct here. If you see
> FileSetDelete->PathNameDeleteTemporaryFile, it already tolerates that
> the file doesn't exist but gives an error only when it is unable to
> link. So, with this missing_ok users (say like worker.c) won't even
> get errors when they are not able to remove files whereas I think the
> need for the patch is to not get an error when the file doesn't exist.
> I think you don't need to change anything in the way we invoke
> FileSetDelete.
Right, fixed.
> 2.
> -static HTAB *xidhash = NULL;
> +static FileSet *stream_fileset = NULL;
>
> Can we keep this in LogicalRepWorker and initialize it accordingly?
Done
> 3.
> + /* Open the subxact file, if it does not exist, create it. */
> + fd = BufFileOpenFileSet(stream_fileset, path, O_RDWR, true);
> + if (fd == NULL)
> + fd = BufFileCreateFileSet(stream_fileset, path);
>
> I think retaining the existing comment: "Create the subxact file if it
> not already created, otherwise open the existing file." seems better
> here.
Done
> 4.
> /*
> - * If there is no subtransaction then nothing to do, but if already have
> - * subxact file then delete that.
> + * If there are no subtransactions, there is nothing to be done, but if
> + * subxacts already exist, delete it.
> */
>
> How about changing the above comment to something like: "Delete the
> subxacts file, if exists"?
Done
> 5. Can we slightly change the commit message as:
> Optimize fileset usage in apply worker.
>
> Use one fileset for the entire worker lifetime instead of using
> separate filesets for each streaming transaction. Now, the
> changes/subxacts files for every streaming transaction will be created
> under the same fileset and the files will be deleted after the
> transaction is completed.
>
> This patch extends the BufFileOpenFileSet and BufFileDeleteFileSet
> APIs to allow users to specify whether to give an error on missing
> files.
Done
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com