public inbox for [email protected]
help / color / mirror / Atom feedFrom: Chao Li <[email protected]>
To: Paul A Jungwirth <[email protected]>
Cc: Peter Eisentraut <[email protected]>
Cc: jian he <[email protected]>
Cc: SATYANARAYANA NARLAPURAM <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: FOR PORTION OF does not recompute GENERATED STORED columns that depend on the range column
Date: Sat, 9 May 2026 14:38:03 +0800
Message-ID: <[email protected]> (raw)
In-Reply-To: <CA+renyUTzwAMar173cbbxJypChp7s=txxgB+LYJQ5oRZ3a5hYQ@mail.gmail.com>
References: <CAHg+QDcd=t69gLf9yQexO07EJ2mx0Z70NFHo6h94X1EDA=hM0g@mail.gmail.com>
<CACJufxGreOtA-S-qeHyS5iSSsj5zZX0W3Rf8FxbyL+SVXFjLYw@mail.gmail.com>
<CAHg+QDeGLfz8YSCChjqrxaVSrz9AnMA0NrmsNogLqeGgCt7-wg@mail.gmail.com>
<CA+renyWD+XXifwswE74vhjooqbiVKu4qVhLvpMcUQBzrjVjT7A@mail.gmail.com>
<CACJufxHYntqy2fo9CFWDDrqKjcMK8DGRM3kse4YnXYnPYq2Hiw@mail.gmail.com>
<CA+renyVp4rgj8x0ERXRkZp223eyBZ_XZr2RVCXvjzKBhTtS6Yw@mail.gmail.com>
<CACJufxEkomKYmWgqXJmQr_qS+z=BZ3w801eh7Z7ekh-3oHXxHQ@mail.gmail.com>
<CA+renyWk7kVsZJPZKzN95mYkO7S=hDUx=+fUPtbg9qFqeepCpg@mail.gmail.com>
<CACJufxHai+HB1gkNqVEHe4oKyUmXfAagWBYAWXYKy8hyMV3RxA@mail.gmail.com>
<CA+renyUBQdhnYxfPay+dxFs6BU1-fnEQskT0r-3dQ2v-ZnmZzg@mail.gmail.com>
<CA+renyVZLYSghHb_85w0pUBG0KNGKTwciFTKBK5--rpHUM+VdA@mail.gmail.com>
<[email protected]>
<CA+renyU6rNkiNGreMyQ7pU_F6-5RND5jchHbECH4NoRO7W0Q-Q@mail.gmail.com>
<[email protected]>
<[email protected]>
<[email protected]>
<CA+renyUBLAynaj0BKhajB6F=sLuitQkjT+_sOt5HBSRn82iQsw@mail.gmail.com>
<[email protected]>
<CA+renyUTzwAMar173cbbxJypChp7s=txxgB+LYJQ5oRZ3a5hYQ@mail.gmail.com>
> On May 8, 2026, at 23:25, Paul A Jungwirth <[email protected]> wrote:
>
> On Fri, May 8, 2026 at 12:10 AM Chao Li <[email protected]> wrote:
>>> <v11-0001-Fix-FOR-PORTION-OF-column-dependency-tracking.patch><v11-0002-Fix-FOR-PORTION-OF-with-partitions-and-inheritan.patch>
>>
>> Thanks for updating the patch and making the separation. After reading v11, I still have a few comments for 0001.
>>
>> ```
>> + if (relinfo->ri_forPortionOf)
>> + {
>> + AttrNumber rangeAttno = relinfo->ri_forPortionOf->fp_rangeAttno;
>> +
>> + if (!bms_is_member(rangeAttno - FirstLowInvalidHeapAttributeNumber,
>> + updatedCols))
>> + {
>> + MemoryContext oldContext;
>> +
>> + oldContext = MemoryContextSwitchTo(estate->es_query_cxt);
>> +
>> + updatedCols = bms_copy(updatedCols);
>> + updatedCols =
>> + bms_add_member(updatedCols,
>> + rangeAttno - FirstLowInvalidHeapAttributeNumber);
>> +
>> + MemoryContextSwitchTo(oldContext);
>> + }
>> }
>> ```
>>
>> 1. I don’t think we should unconditionally do bms_copy, only if (updatedCols == perminfo->updatedCols), we need to make the copy.
>
> You're saying we can skip the copy if execute_attr_map_cols already
> made a new bms above. That's true. Since we're going to just use the
> current memory context (see below), that seems safe.
>
>> 2. I doubt if we need to switch to estate->es_query_cxt. Because ExecGetUpdatedCols() is called by ExecGetAllUpdatedCols(), and its header comment says the function runs in per-tuple memory context:
>> ```
>> /*
>> * Return columns being updated, including generated columns
>> *
>> * The bitmap is allocated in per-tuple memory context. It's up to the caller to
>> * copy it into a different context with the appropriate lifespan, if needed.
>> */
>> Bitmapset *
>> ExecGetAllUpdatedCols(ResultRelInfo *relinfo, EState *estate)
>> ```
>>
>> So I think bms_copy and bms_add_member should be just done in the current memory context.
>
> Okay. I think using the current memory context is more correct anyway.
> There are other callers, and using the query memory context isn't
> necessarily what they want. Also the bms (potentially) allocated by
> execute_attr_map_cols is in the current memory context, so doing
> something different feels surprising. And it's safer not to change the
> behavior. Maybe there is a memory leak because of a long-lived
> context, but then it exists already. I added a comment to
> ExecGetUpdatedCols to call out that we use the current memory context.
>
>> 3. "rangeAttno - FirstLowInvalidHeapAttributeNumber” appears twice, maybe add a local variable to avoid the duplication.
>
> Okay.
>
> v12 attached.
>
> Yours,
>
> --
> Paul ~{:-)
> [email protected]
> <v12-0001-Fix-FOR-PORTION-OF-column-dependency-tracking.patch><v12-0002-Fix-FOR-PORTION-OF-with-partitions-and-inheritan.patch>
Thanks for updating the patch. I have no more comment. V12 LGTM.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: FOR PORTION OF does not recompute GENERATED STORED columns that depend on the range column
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox