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 3:34 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> 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.

Thank you for updating the patch!

The patch looks good to me except for the below comment:

+        /* Delete the subxact file, if it exist. */
+        subxact_filename(path, subid, xid);
+        BufFileDeleteFileSet(stream_fileset, path, true);

s/it exist/it exists/

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/