Re: Forget close an open relation in ReorderBufferProcessTXN()

On Mon, May 17, 2021 at 9:45 PM osumi(dot)takamichi(at)fujitsu(dot)com
<osumi(dot)takamichi(at)fujitsu(dot)com> wrote:
> On Monday, May 17, 2021 6:52 PM Amit Langote <amitlangote09(at)gmail(dot)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