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

On Fri, Aug 27, 2021 at 10:56 AM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
> On Thu, Aug 26, 2021 2:18 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> > The patch looks good to me, I have rebased 0002 atop
> > this patch and also done some cosmetic fixes in 0002.
>
> Here are some comments for the 0002 patch.
>
> 1)
>
> - * toplevel transaction. Each subscription has a separate set of files.
> + * toplevel transaction. Each subscription has a separate files.
>
> a separate files => a separate file

Done

> 2)
> +        * Otherwise, just open it file for writing, in append mode.
>          */
>
> open it file => open the file

Done

>
> 3)
>         if (subxact_data.nsubxacts == 0)
>         {
> -               if (ent->subxact_fileset)
> -               {
> -                       cleanup_subxact_info();
> -                       FileSetDeleteAll(ent->subxact_fileset);
> -                       pfree(ent->subxact_fileset);
> -                       ent->subxact_fileset = NULL;
> -               }
> +               cleanup_subxact_info();
> +               BufFileDeleteFileSet(stream_fileset, path, true);
> +
>
> Before applying the patch, the code only invoke cleanup_subxact_info() when the
> file exists. After applying the patch, it will invoke cleanup_subxact_info()
> either the file exists or not. Is it correct ?

I think this is just structure resetting at the end of the stream.
Earlier the hash was telling us whether we have ever dirtied that
structure or not but now we are not maintaining that hash so we just
reset it at the end of the stream.  I don't think its any bad, in fact
I think this is much cheaper compared to maining the hash.

>
> 4)
>         /*
> -        * If this is the first streamed segment, the file must not exist, so make
> -        * sure we're the ones creating it. Otherwise just open the file for
> -        * writing, in append mode.
> +        * If this is the first streamed segment, create the changes file.
> +        * Otherwise, just open it file for writing, in append mode.
>          */
>         if (first_segment)
> -       {
>         ...
> -               if (found)
> -                       ereport(ERROR,
> -                                       (errcode(ERRCODE_PROTOCOL_VIOLATION),
> -                                        errmsg_internal("incorrect first-segment flag for streamed replication
transaction")));
>         ...
> -       }
> +               stream_fd = BufFileCreateFileSet(stream_fileset, path);
>
>
> Since the function BufFileCreateFileSet() doesn't check the file's existence,
> the change here seems remove the file existence check which the old code did.

Not really, we were just doing a sanity check of the in memory hash
entry, now we don't maintain that so we don't need to do that.

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