Horton

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Use relation_close() more consistently in contrib/

  2. Use table/index_close() more consistently

  3. pageinspect: use index_close() for GiST index relation

  1. Fix memory leak in gist_page_items() of pageinspect

    Chao Li <li.evan.chao@gmail.com> — 2025-12-12T08:50:09Z

    Hi Hackers,

While reading the code of pageinspect, I just found a memory leak
in gist_page_items():

```
   values[4] = CStringGetTextDatum(buf.data);
   nulls[4] = false;
```

where CStringGetTextDatum() has made a copy of buf.data and assigned to
value[4], however buf.data is never free-ed. This leak is inside a
per-tuple loop, thus it should be fixed.

In the meantime, the other small issue was also found in the same function.
An index is opened by index_open() but closed by index_close() and
relation_close() in different places. I also fixed the problem by changing
relation_close() to index_close().

Best regards,
==
Chao Li (Evan)
---------------------
HighGo Software Co., Ltd.
https://www.highgo.com/

  2. Re: Fix memory leak in gist_page_items() of pageinspect

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-12-12T09:00:08Z

    Hi,

On Fri, Dec 12, 2025 at 04:50:09PM +0800, Chao Li wrote:
> Hi Hackers,
> 
> While reading the code of pageinspect, I just found a memory leak
> in gist_page_items():
> 
> ```
>    values[4] = CStringGetTextDatum(buf.data);
>    nulls[4] = false;
> ```
> 
> where CStringGetTextDatum() has made a copy of buf.data and assigned to
> value[4], however buf.data is never free-ed.

I did not look in details but I think that we should be in a short lived
memory context here so we generally prefer to avoid using pfree for those cases.

> This leak is inside a
> per-tuple loop, thus it should be fixed.

That might be a valid reason though. Do you have an idea of the "leak" size
based on the number of tuples?

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  3. Re: Fix memory leak in gist_page_items() of pageinspect

    Michael Paquier <michael@paquier.xyz> — 2025-12-12T09:48:09Z

    On Fri, Dec 12, 2025 at 09:00:08AM +0000, Bertrand Drouvot wrote:
> On Fri, Dec 12, 2025 at 04:50:09PM +0800, Chao Li wrote:
>> where CStringGetTextDatum() has made a copy of buf.data and assigned to
>> value[4], however buf.data is never free-ed.
> 
> I did not look in details but I think that we should be in a short lived
> memory context here so we generally prefer to avoid using pfree for those cases.

The only thing that does a memory allocation is the StringInfo, why
would a memory context be worth the complication here?

> That might be a valid reason though. Do you have an idea of the "leak" size
> based on the number of tuples?

I presume that this just needs to imply a very large index, as we are
doing a simple loop with the items stored in a tuplestore
(CStringGetTextDatum does a palloc() for a value so we do not care
about the contents of the StringInfo).

The relation_close() inconsistency is a fun find.  We tend to be
careful with the APIs when opening relations and the ones that enforce
relkind checks, at least on style ground.
--
Michael

  4. Re: Fix memory leak in gist_page_items() of pageinspect

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-12-12T10:23:48Z

    Hi,

On Fri, Dec 12, 2025 at 06:48:09PM +0900, Michael Paquier wrote:
> On Fri, Dec 12, 2025 at 09:00:08AM +0000, Bertrand Drouvot wrote:
> > On Fri, Dec 12, 2025 at 04:50:09PM +0800, Chao Li wrote:
> >> where CStringGetTextDatum() has made a copy of buf.data and assigned to
> >> value[4], however buf.data is never free-ed.
> > 
> > I did not look in details but I think that we should be in a short lived
> > memory context here so we generally prefer to avoid using pfree for those cases.
> 
> The only thing that does a memory allocation is the StringInfo, why
> would a memory context be worth the complication here?

I meant to say: we are probably already in a short lived memory context. I did
not mean to say to switch to a new one.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  5. Re: Fix memory leak in gist_page_items() of pageinspect

    Heikki Linnakangas <hlinnaka@iki.fi> — 2025-12-12T10:27:52Z

    On 12/12/2025 11:48, Michael Paquier wrote:
> On Fri, Dec 12, 2025 at 09:00:08AM +0000, Bertrand Drouvot wrote:
>> On Fri, Dec 12, 2025 at 04:50:09PM +0800, Chao Li wrote:
>>> where CStringGetTextDatum() has made a copy of buf.data and assigned to
>>> value[4], however buf.data is never free-ed.
>>
>> I did not look in details but I think that we should be in a short lived
>> memory context here so we generally prefer to avoid using pfree for those cases.
> 
> The only thing that does a memory allocation is the StringInfo, why
> would a memory context be worth the complication here?
> 
>> That might be a valid reason though. Do you have an idea of the "leak" size
>> based on the number of tuples?
> 
> I presume that this just needs to imply a very large index, as we are
> doing a simple loop with the items stored in a tuplestore
> (CStringGetTextDatum does a palloc() for a value so we do not care
> about the contents of the StringInfo).

The function is executed in a short lived ExprContext anyway. It's reset 
per row. So if you do something:

select * from
   generate_series(1, 10) g,
   gist_page_items(get_raw_page('gist_idx', g), 'gist_idx');

the memory context is reset between each row, that is, between every 
index page.

It might be nice to pfree it anyway, even though it doesn't accumulate 
across calls. If you have 100 tuples on an index page, it uses 100 kB of 
memory for no good reason.

If we're going to bother changing this at all, let's consider reusing 
the buffer. So instead of initStringInfo()+pfree on every tuple, 
allocate it once and use resetStringInfo().

> The relation_close() inconsistency is a fun find.  We tend to be
> careful with the APIs when opening relations and the ones that enforce
> relkind checks, at least on style ground.

Yeah, that we should fix, for the sake of consistency.

- Heikki

  6. Re: Fix memory leak in gist_page_items() of pageinspect

    Daniel Gustafsson <daniel@yesql.se> — 2025-12-12T10:29:27Z

    > On 12 Dec 2025, at 11:27, Heikki Linnakangas <hlinnaka@iki.fi> wrote:

> If we're going to bother changing this at all, let's consider reusing the buffer. So instead of initStringInfo()+pfree on every tuple, allocate it once and use resetStringInfo().

+1

--
Daniel Gustafsson

  7. Re: Fix memory leak in gist_page_items() of pageinspect

    Chao Li <li.evan.chao@gmail.com> — 2025-12-13T02:37:40Z

    On Dec 12, 2025, at 18:29, Daniel Gustafsson <daniel@yesql.se> wrote:

On 12 Dec 2025, at 11:27, Heikki Linnakangas <hlinnaka@iki.fi> wrote:


If we're going to bother changing this at all, let's consider reusing the
buffer. So instead of initStringInfo()+pfree on every tuple, allocate it
once and use resetStringInfo().


+1



It looks like there’s no disagreement on the index_close() fix, while the
StringInfo change has triggered some discussion.

So I’ve split the fix into two commits, which I probably should have done
from the beginning. 0001 contains the index_close() fix, and 0002 contains
the StringInfo fix.

As Heikki suggested, I switched to using resetStringInfo() in 0002. One
thing I’d like to highlight is that this function only uses the StringInfo
buffer under certain conditions. If we initialize the StringInfo
unconditionally, that feels wasteful; if we initialize it lazily, as in
0002, then we need some extra checks to manage its state.


Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

  8. Re: Fix memory leak in gist_page_items() of pageinspect

    Michael Paquier <michael@paquier.xyz> — 2025-12-15T04:08:44Z

    On Sat, Dec 13, 2025 at 10:37:40AM +0800, Chao Li wrote:
> So I’ve split the fix into two commits, which I probably should have done
> from the beginning. 0001 contains the index_close() fix, and 0002 contains
> the StringInfo fix.

While passing through, I have applied 0001.  (Note that the email
address you have added to the commit message of 0001 did not match
with your actual email address, perhaps you did not intend to do
that..)
--
Michael

  9. Re: Fix memory leak in gist_page_items() of pageinspect

    Chao Li <li.evan.chao@gmail.com> — 2025-12-15T05:45:10Z

    
> On Dec 15, 2025, at 12:08, Michael Paquier <michael@paquier.xyz> wrote:
> 
> (Note that the email
> address you have added to the commit message of 0001 did not match
> with your actual email address, perhaps you did not intend to do
> that..)

Hi Michael,

Yes, that was intentional. lic@highgo.com is my company email address, and I’m supposed to use it when participating in community activities. However, it doesn’t work well with the Mail Archive, so I have to use my personal Gmail address there. That said, for patches that I initiate, I intentionally use my company email address.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

  10. Re: Fix memory leak in gist_page_items() of pageinspect

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-12-15T08:12:28Z

    Hi,

On Sat, Dec 13, 2025 at 10:37:40AM +0800, Chao Li wrote:
> On Dec 12, 2025, at 18:29, Daniel Gustafsson <daniel@yesql.se> wrote:
> 
> On 12 Dec 2025, at 11:27, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> 
> 
> If we're going to bother changing this at all, let's consider reusing the
> buffer. So instead of initStringInfo()+pfree on every tuple, allocate it
> once and use resetStringInfo().
> 
> 
> +1
> 
> As Heikki suggested, I switched to using resetStringInfo() in 0002. One
> thing I’d like to highlight is that this function only uses the StringInfo
> buffer under certain conditions. If we initialize the StringInfo
> unconditionally, that feels wasteful; if we initialize it lazily, as in
> 0002, then we need some extra checks to manage its state.

I think that 0002 works fine.

I'm just wondering if:

+       StringInfoData buf = {0};       /* mark as uninitialized */

does not "break" the semantic of "maxlen == 0" means "read-only".

According to the StringInfoData definition comments:

"
 * As a special case, a StringInfoData can be initialized with a read-only
 * string buffer.  In this case "data" does not necessarily point at a
 * palloc'd chunk, and management of the buffer storage is the caller's
 * responsibility.  maxlen is set to zero to indicate that this is the case.
 * Read-only StringInfoDatas cannot be appended to or reset.
 * Also, it is caller's option whether a read-only string buffer has a
 * terminating '\0' or not.  This depends on the intended usage."

The patch uses maxlen == 0 to detect "uninitialized", but the documentation
explicitly says maxlen == 0 indicates "read-only". Maybe adjust the doc a bit or
add a buf_initialized bool in gist_page_items instead?

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  11. Re: Fix memory leak in gist_page_items() of pageinspect

    Chao Li <li.evan.chao@gmail.com> — 2025-12-15T09:28:45Z

    On Mon, Dec 15, 2025 at 4:12 PM Bertrand Drouvot <
bertranddrouvot.pg@gmail.com> wrote:

> Hi,
>
> On Sat, Dec 13, 2025 at 10:37:40AM +0800, Chao Li wrote:
> > On Dec 12, 2025, at 18:29, Daniel Gustafsson <daniel@yesql.se> wrote:
> >
> > On 12 Dec 2025, at 11:27, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> >
> >
> > If we're going to bother changing this at all, let's consider reusing the
> > buffer. So instead of initStringInfo()+pfree on every tuple, allocate it
> > once and use resetStringInfo().
> >
> >
> > +1
> >
> > As Heikki suggested, I switched to using resetStringInfo() in 0002. One
> > thing I’d like to highlight is that this function only uses the
> StringInfo
> > buffer under certain conditions. If we initialize the StringInfo
> > unconditionally, that feels wasteful; if we initialize it lazily, as in
> > 0002, then we need some extra checks to manage its state.
>
> I think that 0002 works fine.
>
> I'm just wondering if:
>
> +       StringInfoData buf = {0};       /* mark as uninitialized */
>
> does not "break" the semantic of "maxlen == 0" means "read-only".
>
> According to the StringInfoData definition comments:
>
> "
>  * As a special case, a StringInfoData can be initialized with a read-only
>  * string buffer.  In this case "data" does not necessarily point at a
>  * palloc'd chunk, and management of the buffer storage is the caller's
>  * responsibility.  maxlen is set to zero to indicate that this is the
> case.
>  * Read-only StringInfoDatas cannot be appended to or reset.
>  * Also, it is caller's option whether a read-only string buffer has a
>  * terminating '\0' or not.  This depends on the intended usage."
>
> The patch uses maxlen == 0 to detect "uninitialized", but the documentation
> explicitly says maxlen == 0 indicates "read-only". Maybe adjust the doc a
> bit or
> add a buf_initialized bool in gist_page_items instead?
>
>
Hi Bertrand,

I think your point is valid. Let's not break the current meaning of maxlen.
I added buf_initialized in v3. As 0001 has been pushed, v2-0002 now becomes
v3-0001.

Best regards,
Chao Li (Evan)
---------------------
HighGo Software Co., Ltd.
https://www.highgo.com/

  12. Re: Fix memory leak in gist_page_items() of pageinspect

    Chao Li <li.evan.chao@gmail.com> — 2025-12-15T10:56:06Z

    On Mon, Dec 15, 2025 at 5:28 PM Chao Li <li.evan.chao@gmail.com> wrote:

>
> On Mon, Dec 15, 2025 at 4:12 PM Bertrand Drouvot <
> bertranddrouvot.pg@gmail.com> wrote:
>
>> Hi,
>>
>> On Sat, Dec 13, 2025 at 10:37:40AM +0800, Chao Li wrote:
>> > On Dec 12, 2025, at 18:29, Daniel Gustafsson <daniel@yesql.se> wrote:
>> >
>> > On 12 Dec 2025, at 11:27, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>> >
>> >
>> > If we're going to bother changing this at all, let's consider reusing
>> the
>> > buffer. So instead of initStringInfo()+pfree on every tuple, allocate it
>> > once and use resetStringInfo().
>> >
>> >
>> > +1
>> >
>> > As Heikki suggested, I switched to using resetStringInfo() in 0002. One
>> > thing I’d like to highlight is that this function only uses the
>> StringInfo
>> > buffer under certain conditions. If we initialize the StringInfo
>> > unconditionally, that feels wasteful; if we initialize it lazily, as in
>> > 0002, then we need some extra checks to manage its state.
>>
>> I think that 0002 works fine.
>>
>> I'm just wondering if:
>>
>> +       StringInfoData buf = {0};       /* mark as uninitialized */
>>
>> does not "break" the semantic of "maxlen == 0" means "read-only".
>>
>> According to the StringInfoData definition comments:
>>
>> "
>>  * As a special case, a StringInfoData can be initialized with a read-only
>>  * string buffer.  In this case "data" does not necessarily point at a
>>  * palloc'd chunk, and management of the buffer storage is the caller's
>>  * responsibility.  maxlen is set to zero to indicate that this is the
>> case.
>>  * Read-only StringInfoDatas cannot be appended to or reset.
>>  * Also, it is caller's option whether a read-only string buffer has a
>>  * terminating '\0' or not.  This depends on the intended usage."
>>
>> The patch uses maxlen == 0 to detect "uninitialized", but the
>> documentation
>> explicitly says maxlen == 0 indicates "read-only". Maybe adjust the doc a
>> bit or
>> add a buf_initialized bool in gist_page_items instead?
>>
>>
> Hi Bertrand,
>
> I think your point is valid. Let's not break the current meaning of
> maxlen. I added buf_initialized in v3. As 0001 has been pushed, v2-0002 now
> becomes v3-0001.
>
> Oops, forgot the attachment. Here comes it.

Best regards,
Chao Li (Evan)
---------------------
HighGo Software Co., Ltd.
https://www.highgo.com/

  13. Re: Fix memory leak in gist_page_items() of pageinspect

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-12-15T13:39:56Z

    Hi,

On Mon, Dec 15, 2025 at 06:56:06PM +0800, Chao Li wrote:
> On Mon, Dec 15, 2025 at 5:28 PM Chao Li <li.evan.chao@gmail.com> wrote:
> >
> > I think your point is valid. Let's not break the current meaning of
> > maxlen. I added buf_initialized in v3. As 0001 has been pushed, v2-0002 now
> > becomes v3-0001.
> >
> > Oops, forgot the attachment. Here comes it.

LGTM, thanks!

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  14. Re: Fix memory leak in gist_page_items() of pageinspect

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-12-17T11:57:13Z

    Hi,

On Mon, Dec 15, 2025 at 01:08:44PM +0900, Michael Paquier wrote:
> While passing through, I have applied 0001.

Out of curiosity, I searched for other mismatched index_open/relation_close pairs
in the tree and found a few more with the help of [1].

They are fixed in the attached.

Please note that for hash_bitmap_info() and pgstathashindex() the open calls are
changed instead. For those we keep the IS_INDEX() checks to reject partitioned
indexes (which index_open() accepts via validate_relation_kind()). So, that also
changes the error messages in some tests. If we do prefer the previous error
messages we could change the close calls instead (I prefer the way it's done
in the attached though).

Thoughts?

[1]: https://github.com/bdrouvot/coccinelle_on_pg/blob/main/misc/mismatched_open_close_pairs.cocci

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  15. Re: Fix memory leak in gist_page_items() of pageinspect

    Michael Paquier <michael@paquier.xyz> — 2025-12-18T23:01:54Z

    On Wed, Dec 17, 2025 at 11:57:13AM +0000, Bertrand Drouvot wrote:
> Please note that for hash_bitmap_info() and pgstathashindex() the open calls are
> changed instead. For those we keep the IS_INDEX() checks to reject partitioned
> indexes (which index_open() accepts via validate_relation_kind()). So, that also
> changes the error messages in some tests. If we do prefer the previous error
> messages we could change the close calls instead (I prefer the way it's done
> in the attached though).

I have noticed that the two surrounding relation_close() calls for the
parent tables did not get the notice of the change for brin.c of what
you are doing for the indexes, while we use table_open().  I have
fixed these.

It would be nicer if IS_INDEX() could be removed in the other code
paths you are suggesting to change, but the partitioned index argument
also means that we would have two code paths in charge of a relkind
check instead of one.  Just using relation_*() may be cleaner.
--
Michael

  16. Re: Fix memory leak in gist_page_items() of pageinspect

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-12-19T04:29:51Z

    Hi,

On Fri, Dec 19, 2025 at 08:01:54AM +0900, Michael Paquier wrote:
> On Wed, Dec 17, 2025 at 11:57:13AM +0000, Bertrand Drouvot wrote:
> > Please note that for hash_bitmap_info() and pgstathashindex() the open calls are
> > changed instead. For those we keep the IS_INDEX() checks to reject partitioned
> > indexes (which index_open() accepts via validate_relation_kind()). So, that also
> > changes the error messages in some tests. If we do prefer the previous error
> > messages we could change the close calls instead (I prefer the way it's done
> > in the attached though).
> 
> I have noticed that the two surrounding relation_close() calls for the
> parent tables did not get the notice of the change for brin.c of what
> you are doing for the indexes, while we use table_open().  I have
> fixed these.

Nice catch, thanks!

> It would be nicer if IS_INDEX() could be removed in the other code
> paths you are suggesting to change, but the partitioned index argument
> also means that we would have two code paths in charge of a relkind
> check instead of one.  Just using relation_*() may be cleaner.

Yeah, and removing IS_INDEX() and adding a check for partitioned indexes would
still mean 2 code paths. So, v2 changes the close calls (and that's consistent
with what pgstatginindex_internal() is doing.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  17. Re: Fix memory leak in gist_page_items() of pageinspect

    Japin Li <japinli@hotmail.com> — 2025-12-19T06:21:40Z

    On Fri, 19 Dec 2025 at 04:29, Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote:
> Hi,
>
> On Fri, Dec 19, 2025 at 08:01:54AM +0900, Michael Paquier wrote:
>> On Wed, Dec 17, 2025 at 11:57:13AM +0000, Bertrand Drouvot wrote:
>> > Please note that for hash_bitmap_info() and pgstathashindex() the open calls are
>> > changed instead. For those we keep the IS_INDEX() checks to reject partitioned
>> > indexes (which index_open() accepts via validate_relation_kind()). So, that also
>> > changes the error messages in some tests. If we do prefer the previous error
>> > messages we could change the close calls instead (I prefer the way it's done
>> > in the attached though).
>> 
>> I have noticed that the two surrounding relation_close() calls for the
>> parent tables did not get the notice of the change for brin.c of what
>> you are doing for the indexes, while we use table_open().  I have
>> fixed these.
>
> Nice catch, thanks!
>
>> It would be nicer if IS_INDEX() could be removed in the other code
>> paths you are suggesting to change, but the partitioned index argument
>> also means that we would have two code paths in charge of a relkind
>> check instead of one.  Just using relation_*() may be cleaner.
>
> Yeah, and removing IS_INDEX() and adding a check for partitioned indexes would
> still mean 2 code paths. So, v2 changes the close calls (and that's consistent
> with what pgstatginindex_internal() is doing.
>

It would be reasonable to add a comment explaining the choice of
relation_open()/relation_close() instead of the index-specific
index_open()/index_close().

-- 
Regards,
Japin Li
ChengDu WenWu Information Technology Co., Ltd.

  18. Re: Fix memory leak in gist_page_items() of pageinspect

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-12-19T08:23:24Z

    Hi,

On Fri, Dec 19, 2025 at 02:21:40PM +0800, Japin Li wrote:
> On Fri, 19 Dec 2025 at 04:29, Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote:
> > Yeah, and removing IS_INDEX() and adding a check for partitioned indexes would
> > still mean 2 code paths. So, v2 changes the close calls (and that's consistent
> > with what pgstatginindex_internal() is doing.
> >
> 
> It would be reasonable to add a comment explaining the choice of
> relation_open()/relation_close() instead of the index-specific
> index_open()/index_close().

Yeah that would not hurt. What about before the relation_open() calls?

"
Use relation_open() and not index_open() to avoid the validate_relation_kind()
check as we handle relation validation separately below.
"

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  19. Re: Fix memory leak in gist_page_items() of pageinspect

    Japin Li <japinli@hotmail.com> — 2025-12-19T09:36:51Z

    On Fri, 19 Dec 2025 at 08:23, Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote:
> Hi,
>
> On Fri, Dec 19, 2025 at 02:21:40PM +0800, Japin Li wrote:
>> On Fri, 19 Dec 2025 at 04:29, Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote:
>> > Yeah, and removing IS_INDEX() and adding a check for partitioned indexes would
>> > still mean 2 code paths. So, v2 changes the close calls (and that's consistent
>> > with what pgstatginindex_internal() is doing.
>> >
>> 
>> It would be reasonable to add a comment explaining the choice of
>> relation_open()/relation_close() instead of the index-specific
>> index_open()/index_close().
>
> Yeah that would not hurt. What about before the relation_open() calls?
>
> "
> Use relation_open() and not index_open() to avoid the validate_relation_kind()
> check as we handle relation validation separately below.
> "
>

LGTM.

-- 
Regards,
Japin Li
ChengDu WenWu Information Technology Co., Ltd.

  20. Re: Fix memory leak in gist_page_items() of pageinspect

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-12-19T10:26:55Z

    Hi,

On Fri, Dec 19, 2025 at 05:36:51PM +0800, Japin Li wrote:
> On Fri, 19 Dec 2025 at 08:23, Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote:
> > Yeah that would not hurt. What about before the relation_open() calls?
> >
> > "
> > Use relation_open() and not index_open() to avoid the validate_relation_kind()
> > check as we handle relation validation separately below.
> > "
> >
> 
> LGTM.

Thanks! Done that way in the attached.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  21. Re: Fix memory leak in gist_page_items() of pageinspect

    Chao Li <li.evan.chao@gmail.com> — 2025-12-22T02:41:44Z

    
> On Dec 19, 2025, at 18:26, Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote:
> 
> Hi,
> 
> On Fri, Dec 19, 2025 at 05:36:51PM +0800, Japin Li wrote:
>> On Fri, 19 Dec 2025 at 08:23, Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote:
>>> Yeah that would not hurt. What about before the relation_open() calls?
>>> 
>>> "
>>> Use relation_open() and not index_open() to avoid the validate_relation_kind()
>>> check as we handle relation validation separately below.
>>> "
>>> 
>> 
>> LGTM.
> 
> Thanks! Done that way in the attached.
> 
> Regards,
> 
> -- 
> Bertrand Drouvot
> PostgreSQL Contributors Team
> RDS Open Source Databases
> Amazon Web Services: https://aws.amazon.com
> <v3-0001-Use-relation_close-more-consistently.patch>

LGTM. As the immediately following statement is if (!IS_INDEX(rel)), adding a comment to relation_open() feels very necessary.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/