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

On Tue, Aug 24, 2021 at 12:26 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:

> > I was also thinking about the same, does it make sense to name it just
> > ""%s/%s%lu.%u.fileset"?

Done

> I think it is reasonable to use .fileset as proposed by you.
>
> Few other comments:
> =================
> 1.
> + /*
> + * Register before-shmem-exit hook to ensure filesets are dropped while we
> + * can still report stats for underlying temporary files.
> + */
> + before_shmem_exit(worker_cleanup, (Datum) 0);
> +
>
> Do we really need to register a new callback here? Won't the existing
> logical replication worker exit routine (logicalrep_worker_onexit) be
> sufficient for this patch's purpose?

Right, we don't need an extra function for this.

> 2.
> - SharedFileSet *fileset; /* space for segment files if shared */
> - const char *name; /* name of this BufFile if shared */
> + FileSet *fileset; /* space for fileset for fileset based file */
> + const char *name; /* name of this BufFile */
>
> The comments for the above two variables can be written as (a) space
> for fileset based segment files, (b) name of fileset based BufFile.

Done

> 3.
> /*
> - * Create a new segment file backing a shared BufFile.
> + * Create a new segment file backing a fileset BufFile.
> */
> static File
> -MakeNewSharedSegment(BufFile *buffile, int segment)
> +MakeNewSegment(BufFile *buffile, int segment)
>
> I think it is better to name this function as MakeNewFileSetSegment.
> You can slightly change the comment as: "Create a new segment file
> backing a fileset based BufFile."

Make sense

> 4.
> /*
> * It is possible that there are files left over from before a crash
> - * restart with the same name. In order for BufFileOpenShared() not to
> + * restart with the same name. In order for BufFileOpen() not to
> * get confused about how many segments there are, we'll unlink the next
>
> Typo. /BufFileOpen/BufFileOpenFileSet

Fixed

> 5.
> static void
> -SharedSegmentName(char *name, const char *buffile_name, int segment)
> +SegmentName(char *name, const char *buffile_name, int segment)
>
> Can we name this as FileSetSegmentName?

Done

> 6.
> *
> - * The naming scheme for shared BufFiles is left up to the calling code. The
> + * The naming scheme for fileset BufFiles is left up to the calling code.
>
> Isn't it better to say "... fileset based BufFiles .."?

Done

> 7.
> + * FileSets provide a temporary namespace (think directory) so that files can
> + * be discovered by name
>
> A full stop is missing at the end of the statement.

Fixed

> 8.
> + *
> + * The callers are expected to explicitly remove such files by using
> + * SharedFileSetDelete/ SharedFileSetDeleteAll.
> + *
> + * Files will be distributed over the tablespaces configured in
> + * temp_tablespaces.
> + *
> + * Under the covers the set is one or more directories which will eventually
> + * be deleted.
> + */
> +void
> +FileSetInit(FileSet *fileset)
>
> Is there a need to mention 'Shared' in API names (SharedFileSetDelete/
> SharedFileSetDeleteAll) in the comments? Also, there doesn't seem to
> be a need for extra space before *DeleteAll API in comments.

Fixed

> 9.
> * Files will be distributed over the tablespaces configured in
> * temp_tablespaces.
> *
> * Under the covers the set is one or more directories which will eventually
> * be deleted.
> */
> void
> SharedFileSetInit(SharedFileSet *fileset, dsm_segment *seg)
>
> I think we can remove the part of the above comment where it says
> "Files will be distributed over ..." as that is already mentioned atop
> FileSetInit.

Done

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