From: | Amit Langote <amitlangote09(at)gmail(dot)com> |
---|---|
To: | "osumi(dot)takamichi(at)fujitsu(dot)com" <osumi(dot)takamichi(at)fujitsu(dot)com> |
Cc: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, Andres Freund <andres(at)anarazel(dot)de>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Japin Li <japinli(at)hotmail(dot)com>, "pgsql-hackers(at)lists(dot)postgresql(dot)org" <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: Forget close an open relation in ReorderBufferProcessTXN() |
Date: | 2021-05-18 06:30:18 |
Message-ID: | CA+HiwqEzfz34RUMdo=Amu8QrD7bagW0KiMGkFc+z1A_q1D62uA@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
Attachment | Content-Type | Size |
---|---|---|
HEAD-v4-0001-pgoutput-fix-code-for-publishing-using-ancestor-s.patch | application/octet-stream | 5.9 KB |
PG13-v4-0001-pgoutput-fix-code-for-publishing-using-ancestor-s.patch | application/octet-stream | 5.7 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Michael Paquier | 2021-05-18 06:46:07 | Re: pg_get_wal_replay_pause_state() should not return 'paused' while a promotion is ongoing. |
Previous Message | vignesh C | 2021-05-18 05:54:56 | Re: subscriptioncheck failure |