Re: Separate out FileSet from SharedFileSet (was Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o) - Mailing list pgsql-hackers

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.

2.
-static HTAB *xidhash = NULL;
+static FileSet *stream_fileset = NULL;

Can we keep this in LogicalRepWorker and initialize it accordingly?

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.

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

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.

-- 
With Regards,
Amit Kapila.