| From: | Xuneng Zhou <xunengzhou(at)gmail(dot)com> |
|---|---|
| To: | Nazir Bilal Yavuz <byavuz81(at)gmail(dot)com> |
| Cc: | Mihail Nikalayeu <mihailnikalayeu(at)gmail(dot)com>, Andres Freund <andres(at)anarazel(dot)de>, Michael Paquier <michael(at)paquier(dot)xyz>, pgsql-hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
| Subject: | Re: Streamify more code paths |
| Date: | 2026-08-04 07:57:47 |
| Message-ID: | CABPTF7VfDvwnm=VHnSAR=zRQEYZNBAQmuJ51DXt=c5igep8ePQ@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
Hi Bilal,
On Tue, Aug 4, 2026 at 2:40 PM Nazir Bilal Yavuz <byavuz81(at)gmail(dot)com> wrote:
>
> Hi,
>
> On Mon, 3 Aug 2026 at 15:25, Mihail Nikalayeu <mihailnikalayeu(at)gmail(dot)com> wrote:
> >
> > While prototyping with a stress suite, I found memory corruption (and
> > possibly data corruption) issues related to those changes.
> >
> > Issues is related to how the newer bfa3c4f106 affects the old 293e24e507.
> >
> > The 293e24e507 caches the metapage as a pointer, but
> > read_stream_begin_relation may cause that pointer to become freed
> > because of:
> >
> > hashbulkdelete() hash.c:543
> > read_stream_begin_relation(READ_STREAM_MAINTENANCE, ...)
> > read_stream_begin_impl() read_stream.c:806
> > get_tablespace_maintenance_io_concurrency(tablespace_id)
> > get_tablespace() spccache.c
> > SearchSysCache1(TABLESPACEOID, ...) ← cold cache → catalog scan
> > table_open(pg_tablespace)
> > LockRelationOid(1213) lmgr.c:136
> > AcceptInvalidationMessages() ← the flush point
> >
> > causing something like:
> >
> > ERROR: could not open file "base/5/16388.1" (target block 2139062145):
> > previous segment is only 66 blocks
> > CONTEXT: while scanning relation "public.t"
>
> You are right, nice catch!
Thanks for looking into this!
>
> > The simplest fix is to reorder a few lines of code - attached (with a
> > test generated by Claude based on my instructions).
>
> I agree with you, fix LGTM.
After some discussions/debates with Sol, it offers a copying
alternative for fixing the issue:
HashMetaPageData local_metapage;
cachedmetap = _hash_getcachedmetap(rel, ...);
memcpy(&local_metapage, cachedmetap, sizeof(local_metapage));
/* May invalidate and free rd_amcache. */
some_catalog_access();
use(&local_metapage); /* still allocated */
I don't have a strong opinion over them yet. Just putting it on the
table in case the game is over before putting my thoughts together.
> > But probably we should think broader to avoid such issues in the future.
>
> I am not sure how we can enforce this with a code check. Perhaps we
> can add a comment to the read_stream code, but I am not sure how
> effective that would be.
--
Regards,
Xuneng Zhou
HighGo Software Co., Ltd.
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Chao Li | 2026-08-04 08:05:00 | Re: Add a hook for handling logical decoding messages on subscribers. |
| Previous Message | Bharath Rupireddy | 2026-08-04 07:50:00 | Re: enhance wraparound warnings |