MIME-Version: 1.0
References: <ZAaGkCDEl8kC/GWH@paquier.xyz> <20230307.111745.676528378431466705.horikyota.ntt@gmail.com>
 <ZAbPUedT4uURiHTO@paquier.xyz> <20230307.154902.1060895277048303254.horikyota.ntt@gmail.com>
 <ZAblPeGOe4bxY0HE@paquier.xyz> <CALj2ACVoowu=14F9uLSPfT5p0tJRtssAN47p8Kpq55SLPT=ZxQ@mail.gmail.com>
 <ZAg5CzQM4ioRJg/7@paquier.xyz>
In-Reply-To: <ZAg5CzQM4ioRJg/7@paquier.xyz>
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Wed, 8 Mar 2023 16:01:56 +0530
Message-ID: <CALj2ACXR_SWazf5UVTBPa_32rwD2=M4Z1j6qt8k7QND2BEpuVA@mail.gmail.com>
Subject: Re: Add pg_walinspect function with block info columns
To: Michael Paquier <michael@paquier.xyz>
Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>, boekewurm+postgres@gmail.com, 
	melanieplageman@gmail.com, pgsql-hackers@postgresql.org, pg@bowt.ie
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CALj2ACXR_SWazf5UVTBPa_32rwD2%3DM4Z1j6qt8k7QND2BEpuVA%40mail.gmail.com>
Precedence: bulk

On Wed, Mar 8, 2023 at 12:58=E2=80=AFPM Michael Paquier <michael@paquier.xy=
z> wrote:
>
> On Tue, Mar 07, 2023 at 03:56:22PM +0530, Bharath Rupireddy wrote:
> > That would be a lot better. Not just the test, but also the
> > documentation can have it. Simple way to generate such a record (both
> > block data and FPI) is to just change the wal_level to logical in
> > walinspect.conf [1], see code around REGBUF_KEEP_DATA and
> > RelationIsLogicallyLogged in heapam.c
>
> I don't agree that we need to go down to wal_level=3Dlogical for this.
> The important part is to check that the non-NULL and NULL paths for
> the block data and FPI data are both taken, making 4 paths to check.
> So we need two tests at minimum, which would be either:
> - One SQL generating no FPI with no block data and a second generating
> a FPI with block data.  v2 was doing that but did not cover the first
> case.
> - One SQL generating a FPI with no block data and a second generating
> no FPI with block data.
>
> So let's just geenrate a heap record on an UPDATE, for example, like
> in the version attached.

Yup, that should work too because block data gets logged [1].

> > 4. I think we need to free raw_data, raw_page and flags as we loop
> > over multiple blocks (XLR_MAX_BLOCK_ID) and will leak memory which can
> > be a problem if we have many blocks assocated with a single WAL
> > record.
> >             flags =3D (Datum *) palloc0(sizeof(Datum) * bitcnt);
> > Also, we will leak all CStringGetTextDatum memory in the block_id for l=
oop.
> > Another way is to use and reset temp memory context in the for loop
> > over block_ids. I prefer this approach over multiple pfree()s in
> > block_id for loop.
>
> I disagree, this was on purpose in the last version.  This version
> finishes by calling AllocSetContextCreate() and MemoryContextDelete()
> once per *record*, which will not be free, and we are arguing about
> resetting the memory context after scanning up to XLR_MAX_BLOCK_ID
> blocks, or 32 blocks which would go up to 32kB per page in the worst
> case.  That's not going to matter in a large scan for each record, but
> the extra AllocSet*() calls could.  And we basically do the same thing
> on HEAD.

It's not just 32kB per page right? 32*8KB on HEAD (no block data,
flags and CStringGetTextDatum there). With the patch, the number of
pallocs for each block_id =3D 6 CStringGetTextDatum + BLCKSZ (8KB) +
flags (5*size of ptr) + block data_len. In the worst case, all
XLR_MAX_BLOCK_ID can have both FPIs and block data. Furthermore,
imagine if someone initialized their cluster with a higher BLCKSZ (>=3D
8KB), then the memory leak happens noticeably on a lower-end system.

I understand that performance is critical here but we need to ensure
memory is used wisely. Therefore, I'd still vote to free at least the
major contributors here, that is, pfree(raw_data);, pfree(raw_page);
and pfree(flags); right after they are done using. I'm sure pfree()s
don't hurt more than resetting memory context for every block_id.

> Any comments?

I think we need to output block data length (blk->data_len) similar to
fpilen to save users from figuring out how to get the length of a
bytea column. This will also keep block data in sync with FPI info.

[1]
needs_backup =3D (page_lsn <=3D RedoRecPtr);

(gdb) p page_lsn
$2 =3D 21581544
(gdb) p RedoRecPtr
$3 =3D 21484808
(gdb) p needs_backup
$4 =3D false
(gdb)
(gdb) bt
#0  XLogRecordAssemble (rmid=3D10 '\n', info=3D64 '@',
RedoRecPtr=3D21484808, doPageWrites=3Dtrue, fpw_lsn=3D0x7ffde118d640,
    num_fpi=3D0x7ffde118d634, topxid_included=3D0x7ffde118d633) at xloginse=
rt.c:582
#1  0x00005598cd9c3ef7 in XLogInsert (rmid=3D10 '\n', info=3D64 '@') at
xloginsert.c:497
#2  0x00005598cd930452 in log_heap_update (reln=3D0x7f4a4c7cd808,
oldbuf=3D136, newbuf=3D136, oldtup=3D0x7ffde118d820,
    newtup=3D0x5598d00cb098, old_key_tuple=3D0x0,
all_visible_cleared=3Dfalse, new_all_visible_cleared=3Dfalse)
    at heapam.c:8473
#3  0x00005598cd92876e in heap_update (relation=3D0x7f4a4c7cd808,
otid=3D0x7ffde118dab2, newtup=3D0x5598d00cb098, cid=3D0,
    crosscheck=3D0x0, wait=3Dtrue, tmfd=3D0x7ffde118db60,
lockmode=3D0x7ffde118da74) at heapam.c:3741

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