On Mon, Mar 20, 2023 at 8:51 AM Kyotaro Horiguchi
<horikyota....@gmail.com> wrote:
>
> + /* Get block references, if any, otherwise continue. */
> + if (!XLogRecHasAnyBlockRefs(xlogreader))
>
> code does". I feel we don't need the a comment there.
Removed.
> This means GetWALBlockInfo overwrites the last two columns generated
> by GetWalRecordInfo, but I don't think this approach is clean and
> stable. I agree we don't want the final columns in a block info tuple
> but we don't want to duplicate the common code path.
>
> I initially thought we could devide the function into
> GetWALCommonInfo(), GetWALRecordInfo() and GetWALBlockInfo(), but it
> doesn't seem that simple.. In the end, I think we should have separate
> GetWALRecordInfo() and GetWALBlockInfo() that have duplicate
> "values[i++] = .." lines.
Done as per Peter's suggestion (keeping primary key columns first and
having a bit of code duplicated instead of making it complex in the
name of deduplication). Please see the attached v4 patch set.
On Tue, Mar 21, 2023 at 5:04 AM Peter Geoghegan <p...@bowt.ie> wrote:
>
> On Sun, Mar 19, 2023 at 8:21 PM Kyotaro Horiguchi
> <horikyota....@gmail.com> wrote:
> > The documentation has just one section titled "General Functions"
> > which directly contains detailed explation of four functions, making
> > it hard to get clear understanding of the available functions. I
> > considered breaking it down into a few subsections, but that wouldn't
> > look great since most of them would only contain one function.
> > However, I feel it would be helpful to add a list of all functions at
> > the beginning of the section.
>
> I like the idea of sections, even if there is only one function per
> section in some cases.
Hm, -1 for now. Most of the extensions that I have seen don't have
anything like that. If needed, someone can start a separate thread for
such a proposal for all of the extensions.
> I also think that we should add a "Tip" that advises users that they
> may use an "end LSN" that is the largest possible LSN,
> 'FFFFFFFF/FFFFFFFF' to get information about records up until the
> current LSN of the cluster (per commit 5c1b6628).
>
> Is there a straightforward way to get a usable LSN constant for this
> purpose? The simplest way I could come up with quickly is "SELECT
> pg_lsn(2^64.-1)" -- which still isn't very simple. Actually, it might
> be even worse than 'FFFFFFFF/FFFFFFFF', so perhaps we should just use
> that in the docs new "Tip".
Done.
> > I agree that adding a note about the characteristics would helpful to
> > avoid the misuse of pg_get_wal_block_info(). How about something like,
> > "Note that pg_get_wal_block_info() omits records that contains no
> > block references."?
>
> This should be a strict invariant. In other words, it should be part
> of the documented contract of pg_get_wal_block_info and
> pg_get_wal_records_info. The two functions should be defined in terms
> of each other. Their relationship is important.
>
> Users should be able to safely assume that the records that have a
> NULL block_ref according to pg_get_wal_records_info are *precisely*
> those records that won't have any entries within pg_get_wal_block_info
> (assuming that the same LSN range is used with both functions).
> pg_walinspect should explicitly promise this, and promise the
> corollary condition around non-NULL block_ref records. It is a useful
> promise from the point of view of users. It also makes it easier to
> understand what's really going on here without any ambiguity.
>
> I don't completely disagree with Michael about the redundancy. I just
> think that it's worth it on performance grounds. We might want to say
> that directly in the docs, too.
Added a note in the docs.
> Also, if GetWALBlockInfo() is now supposed to only be called when
> XLogRecHasAnyBlockRefs() now then it should probably have an assertion
> to verify the precondition.
Done.
> > I initially thought we could devide the function into
> > GetWALCommonInfo(), GetWALRecordInfo() and GetWALBlockInfo(), but it
> > doesn't seem that simple.. In the end, I think we should have separate
> > GetWALRecordInfo() and GetWALBlockInfo() that have duplicate
> > "values[i++] = .." lines.
>
> I agree. A little redundancy is better when the alternative is fragile
> code, and I'm pretty sure that that applies here -- there won't be
> very many duplicated lines, and the final code will be significantly
> clearer. There can be a comment about keeping GetWALRecordInfo and
> GetWALBlockInfo in sync.
Done.
On Tue, Mar 21, 2023 at 5:21 AM Peter Geoghegan <p...@bowt.ie> wrote:
>
> The new pg_get_wal_block_info outputs columns in an order that doesn't
> seem like the most useful possible order to me. This gives us another
> reason to have separate GetWALRecordInfo and GetWALBlockInfo utility
> functions rather than sharing logic for building output tuples.
>
> Specifically, I think that pg_get_wal_block_info should ouput the
> "primary key" columns first:
>
> reltablespace, reldatabase, relfilenode, blockid, start_lsn, end_lsn
>
> Next comes the columns that duplicate the columns output by
> pg_get_wal_records_info, in the same order as they appear in
> pg_get_wal_records_info. (Obviously this won't include block_ref).
Done.
On Tue, Mar 21, 2023 at 5:30 AM Peter Geoghegan <p...@bowt.ie> wrote:
>
> I think that we should also make the description output column display
> NULLs for those records that don't output any description string. This
> at least includes the "FPI" record type from the "XLOG" rmgr.
> Alternatively, we could find a way of making it show a description.
Done.
Please see the attached v4 patch set addressing all the review comments.
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
From dc2bf1a2109211df4650c635b7094240d5bef65b Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Thu, 23 Mar 2023 11:25:08 +0000
Subject: [PATCH v4] Few optimizations in pg_walinspect
---
contrib/pg_walinspect/pg_walinspect.c | 58 ++++++++++++++++-----------
1 file changed, 35 insertions(+), 23 deletions(-)
diff --git a/contrib/pg_walinspect/pg_walinspect.c b/contrib/pg_walinspect/pg_walinspect.c
index 3b3215daf5..6b3e71b9cd 100644
--- a/contrib/pg_walinspect/pg_walinspect.c
+++ b/contrib/pg_walinspect/pg_walinspect.c
@@ -187,35 +187,43 @@ GetWALRecordInfo(XLogReaderState *record, Datum *values,
uint32 fpi_len = 0;
StringInfoData rec_desc;
StringInfoData rec_blk_ref;
- uint32 main_data_len;
int i = 0;
- desc = GetRmgr(XLogRecGetRmid(record));
- id = desc.rm_identify(XLogRecGetInfo(record));
-
- if (id == NULL)
- id = psprintf("UNKNOWN (%x)", XLogRecGetInfo(record) & ~XLR_INFO_MASK);
-
- initStringInfo(&rec_desc);
- desc.rm_desc(&rec_desc, record);
-
- /* Block references. */
- initStringInfo(&rec_blk_ref);
- XLogRecGetBlockRefInfo(record, false, true, &rec_blk_ref, &fpi_len);
-
- main_data_len = XLogRecGetDataLen(record);
-
values[i++] = LSNGetDatum(record->ReadRecPtr);
values[i++] = LSNGetDatum(record->EndRecPtr);
values[i++] = LSNGetDatum(XLogRecGetPrev(record));
values[i++] = TransactionIdGetDatum(XLogRecGetXid(record));
+
+ desc = GetRmgr(XLogRecGetRmid(record));
values[i++] = CStringGetTextDatum(desc.rm_name);
+
+ id = desc.rm_identify(XLogRecGetInfo(record));
+ if (id == NULL)
+ id = psprintf("UNKNOWN (%x)", XLogRecGetInfo(record) & ~XLR_INFO_MASK);
+
values[i++] = CStringGetTextDatum(id);
values[i++] = UInt32GetDatum(XLogRecGetTotalLen(record));
- values[i++] = UInt32GetDatum(main_data_len);
+ values[i++] = UInt32GetDatum(XLogRecGetDataLen(record));
+
+ if (XLogRecHasAnyBlockRefs(record))
+ {
+ initStringInfo(&rec_blk_ref);
+ XLogRecGetBlockRefInfo(record, false, true, &rec_blk_ref, &fpi_len);
+ }
+
values[i++] = UInt32GetDatum(fpi_len);
- values[i++] = CStringGetTextDatum(rec_desc.data);
- values[i++] = CStringGetTextDatum(rec_blk_ref.data);
+
+ initStringInfo(&rec_desc);
+ desc.rm_desc(&rec_desc, record);
+ if (rec_desc.len > 0)
+ values[i++] = CStringGetTextDatum(rec_desc.data);
+ else
+ nulls[i++] = true;
+
+ if (XLogRecHasAnyBlockRefs(record))
+ values[i++] = CStringGetTextDatum(rec_blk_ref.data);
+ else
+ nulls[i++] = true;
Assert(i == ncols);
}
@@ -377,6 +385,11 @@ pg_get_wal_block_info(PG_FUNCTION_ARGS)
while (ReadNextXLogRecord(xlogreader) &&
xlogreader->EndRecPtr <= end_lsn)
{
+ CHECK_FOR_INTERRUPTS();
+
+ if (!XLogRecHasAnyBlockRefs(xlogreader))
+ continue;
+
/* Use the tmp context so we can clean up after each tuple is done */
old_cxt = MemoryContextSwitchTo(tmp_cxt);
@@ -385,8 +398,6 @@ pg_get_wal_block_info(PG_FUNCTION_ARGS)
/* clean up and switch back */
MemoryContextSwitchTo(old_cxt);
MemoryContextReset(tmp_cxt);
-
- CHECK_FOR_INTERRUPTS();
}
MemoryContextDelete(tmp_cxt);
@@ -483,8 +494,6 @@ GetWALRecordsInfo(FunctionCallInfo fcinfo, XLogRecPtr start_lsn,
#define PG_GET_WAL_RECORDS_INFO_COLS 11
XLogReaderState *xlogreader;
ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo;
- Datum values[PG_GET_WAL_RECORDS_INFO_COLS] = {0};
- bool nulls[PG_GET_WAL_RECORDS_INFO_COLS] = {0};
MemoryContext old_cxt;
MemoryContext tmp_cxt;
@@ -501,6 +510,9 @@ GetWALRecordsInfo(FunctionCallInfo fcinfo, XLogRecPtr start_lsn,
while (ReadNextXLogRecord(xlogreader) &&
xlogreader->EndRecPtr <= end_lsn)
{
+ Datum values[PG_GET_WAL_RECORDS_INFO_COLS] = {0};
+ bool nulls[PG_GET_WAL_RECORDS_INFO_COLS] = {0};
+
/* Use the tmp context so we can clean up after each tuple is done */
old_cxt = MemoryContextSwitchTo(tmp_cxt);
--
2.34.1
From 72f20a1914c9b02a36d78364b8865913175bb881 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Thu, 23 Mar 2023 17:10:07 +0000
Subject: [PATCH v4] Emit WAL record info via pg_get_wal_block_info
---
.../pg_walinspect/pg_walinspect--1.0--1.1.sql | 12 ++-
contrib/pg_walinspect/pg_walinspect.c | 58 ++++++++---
doc/src/sgml/pgwalinspect.sgml | 99 ++++++++++++-------
3 files changed, 119 insertions(+), 50 deletions(-)
diff --git a/contrib/pg_walinspect/pg_walinspect--1.0--1.1.sql b/contrib/pg_walinspect/pg_walinspect--1.0--1.1.sql
index 586c3b4467..f767211fed 100644
--- a/contrib/pg_walinspect/pg_walinspect--1.0--1.1.sql
+++ b/contrib/pg_walinspect/pg_walinspect--1.0--1.1.sql
@@ -12,12 +12,20 @@ DROP FUNCTION pg_get_wal_stats_till_end_of_wal(pg_lsn, boolean);
--
CREATE FUNCTION pg_get_wal_block_info(IN start_lsn pg_lsn,
IN end_lsn pg_lsn,
- OUT lsn pg_lsn,
- OUT blockid int2,
OUT reltablespace oid,
OUT reldatabase oid,
OUT relfilenode oid,
OUT relblocknumber int8,
+ OUT blockid int2,
+ OUT start_lsn pg_lsn,
+ OUT end_lsn pg_lsn,
+ OUT prev_lsn pg_lsn,
+ OUT xid xid,
+ OUT resource_manager text,
+ OUT record_type text,
+ OUT record_length int4,
+ OUT main_data_length int4,
+ OUT description text,
OUT forkname text,
OUT blockdata bytea,
OUT fpi bytea,
diff --git a/contrib/pg_walinspect/pg_walinspect.c b/contrib/pg_walinspect/pg_walinspect.c
index 6b3e71b9cd..d46738774e 100644
--- a/contrib/pg_walinspect/pg_walinspect.c
+++ b/contrib/pg_walinspect/pg_walinspect.c
@@ -177,6 +177,10 @@ ReadNextXLogRecord(XLogReaderState *xlogreader)
/*
* Get a single WAL record info.
+ *
+ * Note that the per-record information fetching code is same in both
+ * GetWALBlockInfo() and GetWALRecordInfo(), it's only the output columns order
+ * that differs. Try to keep this code in sync.
*/
static void
GetWALRecordInfo(XLogReaderState *record, Datum *values,
@@ -230,15 +234,32 @@ GetWALRecordInfo(XLogReaderState *record, Datum *values,
/*
- * Store a set of block information from a single record (FPI and block
- * information).
+ * Get a single WAL record info along with its block information (block data
+ * and FPI).
+ *
+ * Note that the per-record information fetching code is same in both
+ * GetWALBlockInfo() and GetWALRecordInfo(), it's only the output columns order
+ * that differs. Try to keep this code in sync.
*/
static void
GetWALBlockInfo(FunctionCallInfo fcinfo, XLogReaderState *record)
{
-#define PG_GET_WAL_BLOCK_INFO_COLS 11
+#define PG_GET_WAL_BLOCK_INFO_COLS 19
int block_id;
ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo;
+ RmgrData desc;
+ const char *id;
+ StringInfoData rec_desc;
+
+ Assert(XLogRecHasAnyBlockRefs(record));
+
+ desc = GetRmgr(XLogRecGetRmid(record));
+ id = desc.rm_identify(XLogRecGetInfo(record));
+ if (id == NULL)
+ id = psprintf("UNKNOWN (%x)", XLogRecGetInfo(record) & ~XLR_INFO_MASK);
+
+ initStringInfo(&rec_desc);
+ desc.rm_desc(&rec_desc, record);
for (block_id = 0; block_id <= XLogRecMaxBlockId(record); block_id++)
{
@@ -258,12 +279,24 @@ GetWALBlockInfo(FunctionCallInfo fcinfo, XLogReaderState *record)
(void) XLogRecGetBlockTagExtended(record, block_id,
&rnode, &fork, &blkno, NULL);
- values[i++] = LSNGetDatum(record->ReadRecPtr);
- values[i++] = Int16GetDatum(block_id);
values[i++] = ObjectIdGetDatum(blk->rlocator.spcOid);
values[i++] = ObjectIdGetDatum(blk->rlocator.dbOid);
values[i++] = ObjectIdGetDatum(blk->rlocator.relNumber);
values[i++] = Int64GetDatum((int64) blkno);
+ values[i++] = Int16GetDatum(block_id);
+ values[i++] = LSNGetDatum(record->ReadRecPtr);
+ values[i++] = LSNGetDatum(record->EndRecPtr);
+ values[i++] = LSNGetDatum(XLogRecGetPrev(record));
+ values[i++] = TransactionIdGetDatum(XLogRecGetXid(record));
+ values[i++] = CStringGetTextDatum(desc.rm_name);
+ values[i++] = CStringGetTextDatum(id);
+ values[i++] = UInt32GetDatum(XLogRecGetTotalLen(record));
+ values[i++] = UInt32GetDatum(XLogRecGetDataLen(record));
+
+ if (rec_desc.len > 0)
+ values[i++] = CStringGetTextDatum(rec_desc.data);
+ else
+ nulls[i++] = true;
if (fork >= 0 && fork <= MAX_FORKNUM)
values[i++] = CStringGetTextDatum(forkNames[fork]);
@@ -357,11 +390,12 @@ GetWALBlockInfo(FunctionCallInfo fcinfo, XLogReaderState *record)
}
/*
- * Get information about all the blocks saved in WAL records between start
- * and end LSNs. This produces information about the full page images with
- * their relation information, and the data saved in each block associated
- * to a record. Decompression is applied to the full page images, if
- * necessary.
+ * Get info of all WAL records having block references between start LSN and
+ * end LSN. It outputs one row for each WAL record's block reference (note that
+ * a single WAL record can have one or more blocks associated with it). It
+ * outputs WAL record information as well as block information (block data
+ * and/or FPI). Decompression is applied to the full page images, if necessary.
+ * It omits WAL records that contain no block references.
*/
Datum
pg_get_wal_block_info(PG_FUNCTION_ARGS)
@@ -485,7 +519,7 @@ ValidateInputLSNs(XLogRecPtr start_lsn, XLogRecPtr *end_lsn)
}
/*
- * Get info and data of all WAL records between start LSN and end LSN.
+ * Get info of all WAL records between start LSN and end LSN.
*/
static void
GetWALRecordsInfo(FunctionCallInfo fcinfo, XLogRecPtr start_lsn,
@@ -537,7 +571,7 @@ GetWALRecordsInfo(FunctionCallInfo fcinfo, XLogRecPtr start_lsn,
}
/*
- * Get info and data of all WAL records between start LSN and end LSN.
+ * Get info of all WAL records between start LSN and end LSN.
*/
Datum
pg_get_wal_records_info(PG_FUNCTION_ARGS)
diff --git a/doc/src/sgml/pgwalinspect.sgml b/doc/src/sgml/pgwalinspect.sgml
index 9a0241a8d6..13429a2162 100644
--- a/doc/src/sgml/pgwalinspect.sgml
+++ b/doc/src/sgml/pgwalinspect.sgml
@@ -94,9 +94,10 @@ block_ref | blkref #0: rel 1663/5/60221 fork main blk 2
<para>
Gets information of all the valid WAL records between
<replaceable>start_lsn</replaceable> and <replaceable>end_lsn</replaceable>.
- Returns one row per WAL record. If a future
- <replaceable>end_lsn</replaceable> (i.e. ahead of the current LSN of
- the server) is specified, it returns information until the end of WAL.
+ Returns one row per WAL record. If <replaceable>end_lsn</replaceable>
+ specified is in future (i.e. ahead of the current LSN of the server) or
+ <literal>FFFFFFFF/FFFFFFFF</literal> (i.e. highest possible value for
+ <type>pg_lsn</type> type), it returns information until the end of WAL.
The function raises an error if <replaceable>start_lsn</replaceable>
is not available. For example, usage of the function is as follows:
<screen>
@@ -133,11 +134,13 @@ block_ref |
<replaceable>end_lsn</replaceable>. By default, it returns one row per
<replaceable>resource_manager</replaceable> type. When
<replaceable>per_record</replaceable> is set to <literal>true</literal>,
- it returns one row per <replaceable>record_type</replaceable>. If a
- future <replaceable>end_lsn</replaceable> (i.e. ahead of the current
- LSN of the server) is specified, it returns statistics until the end
- of WAL. An error is raised if <replaceable>start_lsn</replaceable> is
- not available. For example, usage of the function is as follows:
+ it returns one row per <replaceable>record_type</replaceable>.
+ If <replaceable>end_lsn</replaceable> specified is in future (i.e. ahead
+ of the current LSN of the server) or <literal>FFFFFFFF/FFFFFFFF</literal>
+ (i.e. highest possible value for <type>pg_lsn</type> type), it returns
+ statistics until the end of WAL. An error is raised if
+ <replaceable>start_lsn</replaceable> is not available. For example, usage
+ of the function is as follows:
<screen>
postgres=# SELECT * FROM pg_get_wal_stats('0/1E847D00', '0/1E84F500')
WHERE count > 0 LIMIT 1 AND
@@ -164,38 +167,62 @@ combined_size_percentage | 2.8634072910530795
<listitem>
<para>
- Gets a copy of the block information stored in WAL records. This includes
- copies of the block data (<literal>NULL</literal> if none) and full page
- images as <type>bytea</type> values (after
- applying decompression when necessary, or <literal>NULL</literal> if none)
- and their information associated with all the valid WAL records between
- <replaceable>start_lsn</replaceable> and
- <replaceable>end_lsn</replaceable>. Returns one row per block registered
- in a WAL record. If a future <replaceable>end_lsn</replaceable> (i.e.
- ahead of the current LSN of the server) is specified, it returns
- statistics until the end of WAL. An error is raised if
- <replaceable>start_lsn</replaceable> is not available. For example,
- usage of the function is as follows:
+ Gets both WAL record and block information of all the valid WAL records
+ between <replaceable>start_lsn</replaceable> and
+ <replaceable>end_lsn</replaceable>. It omits WAL records that contain no
+ block references. The block information includes copies of the block data
+ (<literal>NULL</literal> if none) and full page images as
+ <type>bytea</type> values (after applying decompression when necessary,
+ or <literal>NULL</literal> if none). Returns one row per block registered
+ in a WAL record. If <replaceable>end_lsn</replaceable> specified is in
+ future (i.e. ahead of the current LSN of the server) or
+ <literal>FFFFFFFF/FFFFFFFF</literal> (i.e. highest possible value for
+ <type>pg_lsn</type> type), it returns information until the end of WAL.
+ An error is raised if <replaceable>start_lsn</replaceable> is not
+ available. For example, usage of the function is as follows:
<screen>
-postgres=# SELECT lsn, blockid, reltablespace, reldatabase, relfilenode,
- relblocknumber, forkname,
- substring(blockdata for 24) as block_trimmed,
+postgres=# SELECT reltablespace, reldatabase, relfilenode,
+ relblocknumber, blockid, start_lsn, end_lsn,
+ prev_lsn, xid, resource_manager, record_type,
+ record_length, main_data_length, description,
+ forkname, substring(blockdata for 24) as block_trimmed,
substring(fpi for 24) as fpi_trimmed, fpilen, fpiinfo
- FROM pg_get_wal_block_info('0/1871080', '0/1871440');
--[ RECORD 1 ]--+---------------------------------------------------
-lsn | 0/18712F8
-blockid | 0
-reltablespace | 1663
-reldatabase | 16384
-relfilenode | 16392
-relblocknumber | 0
-forkname | main
-block_trimmed | \x02800128180164000000
-fpi_trimmed | \x0000000050108701000000002c00601f00200420e0020000
-fpilen | 204
-fpiinfo | {HAS_HOLE,APPLY}
+ FROM pg_get_wal_block_info('0/14BF938', '0/14BFBA0');
+-[ RECORD 1 ]----+---------------------------------------------------
+reltablespace | 1663
+reldatabase | 5
+relfilenode | 16391
+relblocknumber | 9
+blockid | 0
+start_lsn | 0/14BFA20
+end_lsn | 0/14BFB38
+prev_lsn | 0/14BF9A8
+xid | 735
+resource_manager | Heap
+record_type | HOT_UPDATE
+record_length | 279
+main_data_length | 14
+description | off 1 xmax 735 flags 0x10 ; new off 5 xmax 0
+forkname | main
+block_trimmed | \x02800128180102000000
+fpi_trimmed | \x00000000d0f84b01000000002c00601f00200420df020000
+fpilen | 204
+fpiinfo | {HAS_HOLE,APPLY}
</screen>
</para>
+ <note>
+ <para>
+ Note that although <function>pg_get_wal_records_info</function>
+ and <function>pg_get_wal_block_info</function> functions return
+ information of WAL records in common, the
+ <function>pg_get_wal_records_info</function> outputs block information
+ (without block data and FPI data) as a text column
+ (<literal>NULL</literal> if none), and
+ <function>pg_get_wal_block_info</function> outputs one row for each block
+ (including block data and FPI data) associated with each WAL record, omitting
+ WAL records that contain no block references.
+ </para>
+ </note>
</listitem>
</varlistentry>
--
2.34.1
