Re: bugfix: when the blocksize is 32k, the function page_header of pageinspect returns negative numbers.

From: Michael Paquier <michael(at)paquier(dot)xyz>
To: Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com>
Cc: Quan Zongliang <quanzongliang(at)yeah(dot)net>, Justin Pryzby <pryzby(at)telsasoft(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: bugfix: when the blocksize is 32k, the function page_header of pageinspect returns negative numbers.
Date: 2021-07-10 11:29:51
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Fri, Jul 09, 2021 at 05:26:37PM +0530, Bharath Rupireddy wrote:
> 1) How about just adding a comment /* support for old extension
> version */ before INT2OID handling?
> + case INT2OID:
> + values[3] = UInt16GetDatum(page->pd_lower);
> + break;

Yes, having a comment to document from which version this is done
would be nice. This is more consistent with the surroundings.

> 2) Do we ever reach the error statement elog(ERROR, "incorrect output
> types");? We have the function either defined with smallint or int, I
> don't think so we ever reach it. Instead, an assertion would work as
> suggested by Micheal.

I would keep an elog() here for the default case. I was referring to
the use of assertions if changing the code into a single switch/case,
with assertions checking that the other arguments have the expected

> 3) Isn't this test case unstable when regression tests are run with a
> different BLCKSZ setting? Or is it okay that some of the other tests
> for pageinspect already outputs page_size, hash_page_stats.
> +SELECT pagesize, version FROM page_header(get_raw_page('test1', 0));
> + pagesize | version
> +----------+---------
> + 8192 | 4

I don't think it matters much, most of the tests of pageinspect
already rely on 8k pages. So let's keep it as-is.

> 4) Can we arrange pageinspect--1.8--1.9.sql into the first line itself?
> +DATA = pageinspect--1.9--1.10.sql \
> + pageinspect--1.8--1.9.sql \

That's a nit, but why not.

> 5) How about using "int4" instead of just "int", just for readability?

Any way is fine, I'd stick with "int" as the other fields used

> 6) How about something like below instead of repetitive switch statements?
> static inline Datum
> get_page_header_attr(TupleDesc desc, int attno, int val)
> {
> Oid atttypid;
> Datum datum;

Nah. It does not seem like an improvement to me in terms of

So I would finish with the attached, close enough to what Quan has
sent upthread.

Attachment Content-Type Size
pageinspect-fields-v3.patch text/x-diff 5.8 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Michael Paquier 2021-07-10 11:32:13 Re: Support kerberos authentication for postgres_fdw
Previous Message Tomas Vondra 2021-07-10 11:06:43 Re: pg_dump new feature: exporting functions only. Bad or good idea ?