On Mon, May 17, 2021 at 9:45 PM osumi.takami...@fujitsu.com <osumi.takami...@fujitsu.com> wrote: > On Monday, May 17, 2021 6:52 PM Amit Langote <amitlangot...@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
HEAD-v4-0001-pgoutput-fix-code-for-publishing-using-ancestor-s.patch
Description: Binary data
PG13-v4-0001-pgoutput-fix-code-for-publishing-using-ancestor-s.patch
Description: Binary data
