On Tuesday, May 18, 2021 3:30 PM Amit Langote <amitlangote09@gmail.com> wrote:
> On Mon, May 17, 2021 at 9:45 PM osumi.takamichi@fujitsu.com
> <osumi.takamichi@fujitsu.com> wrote:
> > On Monday, May 17, 2021 6:52 PM Amit Langote
> <amitlangote09@gmail.com> wrote:
> > > Both patches are attached.
> > The patch for PG13 can be applied to it cleanly and the RT succeeded.
> >
> > I have few really minor comments on your comments in the patch.
> >
> > (1) schema_sent's comment
> >
> > @@ -94,7 +94,8 @@ typedef struct RelationSyncEntry
> >
> > /*
> > * Did we send the schema? If ancestor relid is set, its schema
> must also
> > - * have been sent for this to be true.
> > + * have been sent and the map to convert the relation's tuples into
> the
> > + * ancestor's format created before this can be set to be true.
> > */
> > bool schema_sent;
> > List *streamed_txns; /* streamed toplevel
> transactions with this
> >
> >
> > I suggest to insert a comma between 'created' and 'before'
> > because the sentence is a bit long and confusing.
> >
> > Or, I thought another comment idea for this part, because the original
> > one doesn't care about the cycle of the reset.
> >
> > "To avoid repetition to send the schema, this is set true after its first
> transmission.
> > Reset when any change of the relation definition is possible. If
> > ancestor relid is set, its schema must have also been sent while the
> > map to convert the relation's tuples into the ancestor's format created,
> before this flag is set true."
> >
> > (2) comment in rel_sync_cache_relation_cb()
> >
> > @@ -1190,13 +1208,25 @@ rel_sync_cache_relation_cb(Datum arg, Oid
> relid)
> >
> > HASH_FIND, NULL);
> >
> > /*
> > - * Reset schema sent status as the relation definition may have
> changed.
> > + * Reset schema sent status as the relation definition may have
> changed,
> > + * also freeing any objects that depended on the earlier definition.
> >
> > How about divide this sentence into two sentences like "Reset schema
> > sent status as the relation definition may have changed.
> > Also, free any objects that depended on the earlier definition."
>
> Thanks for reading it over. I have revised comments in a way that hopefully
> addresses your concerns.
Thank you for your fix.
I think the patches look good to me.
Just in case, I'll report that the two patches succeeded
in the RT as expected and from my side,
there's no more suggestions.
Those are ready for committer, I think.
Best Regards,
Takamichi Osumi