RE: Forget close an open relation in ReorderBufferProcessTXN()

On Monday, May 17, 2021 6:52 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> On Mon, May 17, 2021 at 6:13 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
> wrote:
> > On Fri, May 14, 2021 at 12:44 PM Amit Langote
> <amitlangote09(at)gmail(dot)com> wrote:
> > > On Thu, May 13, 2021 at 7:43 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
> wrote:
> > >
> > > > Also, don't we need to free the
> > > > entire map as suggested by me?
> > >
> > > Yes, I had missed that too. Addressed in the updated patch.
> > >
> >
> > + relentry->map = convert_tuples_by_name(indesc, outdesc);
> > + if (relentry->map == NULL)
> > + {
> > + /* Map not necessary, so free the TupleDescs too. */
> > + FreeTupleDesc(indesc); FreeTupleDesc(outdesc); }
> >
> > I think the patch frees these descriptors when the map is NULL but not
> > otherwise because free_conversion_map won't free these descriptors.
>
> You're right. I have fixed that by making the callback free the TupleDescs
> explicitly.
This fix looks correct. Also, RT of v3 didn't fail.

Further, I've checked the point of view of the newly added tests.
As you said that with v1's contents, the test of v2 failed but
we are fine with v2 and v3, which proves that we adjust DDL in a right way.

> > BTW, have you tried this patch in back branches because I think we
> > should backpatch this fix?
>
> I have created a version of the patch for v13, the only older release that has
> this code, and can see that tests, including the newly added one, pass.
>
> 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."

Best Regards,
Takamichi Osumi