Re: Forget close an open relation in ReorderBufferProcessTXN()

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.

While doing so, it occurred to me (maybe not for the first time) that
we are *unnecessarily* doing send_relation_and_attrs() for a relation
if the changes will be published using an ancestor's schema.  In that
case, sending only the ancestor's schema suffices AFAICS.  Changing
the code that way doesn't break any tests.  I propose that we fix that
too.

Updated patches attached.  I've added a commit message to both patches.

-- 
Amit Langote
EDB: http://www.enterprisedb.com