pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feed[HACKERS] wrong t_bits alignment in pageinspect
5+ messages / 3 participants
[nested] [flat]
Commits from this thread:
2018-01-04 0dc5dfcd7a 18869e202b Fix new test case to not be endian-dependent.
2018-01-04 39cfe86195 ad592f4a6e 5ad1b17287 Fix incorrect computations of length of null bitmap in pageinspect.
* [HACKERS] wrong t_bits alignment in pageinspect
@ 2017-12-15 13:53 Maksim Milyutin <milyutinma@gmail.com>
0 siblings, 1 reply; 5+ messages in thread
From: Maksim Milyutin @ 2017-12-15 13:53 UTC (permalink / raw)
To: pgsql-hackers
Hi!
I found out the problem in exposing values of t_bits field from
heap_page_items function.
When the number of attributes in table is multiple of eight, t_bits
column shows double number of bits in which data fields are included.
For example:
# create table tbl(f1 int, f2 int, f3 int, f4 int, f5 int, f6 int, f7
int, f8 int);
# insert into tbl(f1, f8) values (x'f1'::int, 0);
# select t_bits, t_data from heap_page_items(get_raw_page('tbl', 0));
t_bits | t_data
------------------+--------------------
1000000110001111 | \xf100000000000000
I suppose the prefix 10000001 corresponds to real value of t_bits, the
rest part 10001111 - to the lower byte of f1 field of tbl.
Attached patch fixes this issue.
--
Regards,
Maksim Milyutin
Attachments:
pageinspect_tbits_alignment_fix.patch (text/x-patch, 506B) download
diff --git a/contrib/pageinspect/heapfuncs.c b/contrib/pageinspect/heapfuncs.c
index ca4d3f5..fe53a1a 100644
--- a/contrib/pageinspect/heapfuncs.c
+++ b/contrib/pageinspect/heapfuncs.c
@@ -234,7 +234,7 @@ heap_page_items(PG_FUNCTION_ARGS)
int bits_len;
bits_len =
- ((tuphdr->t_infomask2 & HEAP_NATTS_MASK) / 8 + 1) * 8;
+ TYPEALIGN(8, (tuphdr->t_infomask2 & HEAP_NATTS_MASK));
values[11] = CStringGetTextDatum(
bits_to_text(tuphdr->t_bits, bits_len));
}
* Re: [HACKERS] wrong t_bits alignment in pageinspect
@ 2018-01-02 09:33 Andrey Borodin <x4mmm@yandex-team.ru>
parent: Maksim Milyutin <milyutinma@gmail.com>
0 siblings, 1 reply; 5+ messages in thread
From: Andrey Borodin @ 2018-01-02 09:33 UTC (permalink / raw)
To: Maksim Milyutin <milyutinma@gmail.com>; +Cc: pgsql-hackers
Hi!
> 15 дек. 2017 г., в 18:53, Maksim Milyutin <milyutinma@gmail.com> написал(а):
>
> I found out the problem in exposing values of t_bits field from heap_page_items function.
Probably, this [0] place contains similar bug too?
Also, may be macro HeapTupleHeaderGetNatts() will be a little bit easier to read.
Best regards, Andrey Borodin.
[0] https://github.com/postgres/postgres/blob/master/contrib/pageinspect/heapfuncs.c#L439
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: [HACKERS] wrong t_bits alignment in pageinspect
@ 2018-01-04 18:06 Maksim Milyutin <milyutinma@gmail.com>
parent: Andrey Borodin <x4mmm@yandex-team.ru>
0 siblings, 2 replies; 5+ messages in thread
From: Maksim Milyutin @ 2018-01-04 18:06 UTC (permalink / raw)
To: Andrey Borodin <x4mmm@yandex-team.ru>; +Cc: pgsql-hackers
Hi!
02.01.2018 12:33, Andrey Borodin wrote:
>> 15 дек. 2017 г., в 18:53, Maksim Milyutin <milyutinma@gmail.com> написал(а):
>>
>> I found out the problem in exposing values of t_bits field from heap_page_items function.
> Probably, this [0] place contains similar bug too?
>
> [0] https://github.com/postgres/postgres/blob/master/contrib/pageinspect/heapfuncs.c#L439
Yes, it's so. Thanks a lot for notice.
Attached a new version of patch with regression test.
--
Regards,
Maksim Milyutin
Attachments:
pageinspect_tbits_alignment_fix_v2.patch (text/plain, 3.7K) download
diff --git a/contrib/pageinspect/expected/page.out b/contrib/pageinspect/expected/page.out
index 8e15947a81..97af6f5bd2 100644
--- a/contrib/pageinspect/expected/page.out
+++ b/contrib/pageinspect/expected/page.out
@@ -92,3 +92,20 @@ create table test_part1 partition of test_partitioned for values from ( 1 ) to (
select get_raw_page('test_part1', 0); -- get farther and error about empty table
ERROR: block number 0 is out of range for relation "test_part1"
drop table test_partitioned;
+-- check null bitmap alignment for table whose the number of attributes is multiple of 8
+create table test8 (f1 int, f2 int, f3 int, f4 int, f5 int, f6 int, f7 int, f8 int);
+insert into test8(f1, f8) values (x'f1'::int, 0);
+select t_bits, t_data from heap_page_items(get_raw_page('test8', 0));
+ t_bits | t_data
+----------+--------------------
+ 10000001 | \xf100000000000000
+(1 row)
+
+select tuple_data_split('test8'::regclass, t_data, t_infomask, t_infomask2, t_bits)
+ from heap_page_items(get_raw_page('test8', 0));
+ tuple_data_split
+-------------------------------------------------------------
+ {"\\xf1000000",NULL,NULL,NULL,NULL,NULL,NULL,"\\x00000000"}
+(1 row)
+
+drop table test8;
diff --git a/contrib/pageinspect/heapfuncs.c b/contrib/pageinspect/heapfuncs.c
index 088254453e..99f409846c 100644
--- a/contrib/pageinspect/heapfuncs.c
+++ b/contrib/pageinspect/heapfuncs.c
@@ -234,7 +234,7 @@ heap_page_items(PG_FUNCTION_ARGS)
int bits_len;
bits_len =
- ((tuphdr->t_infomask2 & HEAP_NATTS_MASK) / 8 + 1) * 8;
+ BITMAPLEN(HeapTupleHeaderGetNatts(tuphdr)) * BITS_PER_BYTE;
values[11] = CStringGetTextDatum(
bits_to_text(tuphdr->t_bits, bits_len));
}
@@ -436,12 +436,12 @@ tuple_data_split(PG_FUNCTION_ARGS)
int bits_str_len;
int bits_len;
- bits_len = (t_infomask2 & HEAP_NATTS_MASK) / 8 + 1;
+ bits_len = BITMAPLEN(t_infomask2 & HEAP_NATTS_MASK) * BITS_PER_BYTE;
if (!t_bits_str)
ereport(ERROR,
(errcode(ERRCODE_DATA_CORRUPTED),
errmsg("argument of t_bits is null, but it is expected to be null and %d character long",
- bits_len * 8)));
+ bits_len)));
bits_str_len = strlen(t_bits_str);
if ((bits_str_len % 8) != 0)
@@ -449,11 +449,11 @@ tuple_data_split(PG_FUNCTION_ARGS)
(errcode(ERRCODE_DATA_CORRUPTED),
errmsg("length of t_bits is not a multiple of eight")));
- if (bits_len * 8 != bits_str_len)
+ if (bits_len != bits_str_len)
ereport(ERROR,
(errcode(ERRCODE_DATA_CORRUPTED),
errmsg("unexpected length of t_bits %u, expected %d",
- bits_str_len, bits_len * 8)));
+ bits_str_len, bits_len)));
/* do the conversion */
t_bits = text_to_bits(t_bits_str, bits_str_len);
diff --git a/contrib/pageinspect/sql/page.sql b/contrib/pageinspect/sql/page.sql
index 493ca9b211..16b4571fbf 100644
--- a/contrib/pageinspect/sql/page.sql
+++ b/contrib/pageinspect/sql/page.sql
@@ -41,3 +41,11 @@ select get_raw_page('test_partitioned', 0); -- error about partitioned table
create table test_part1 partition of test_partitioned for values from ( 1 ) to (100);
select get_raw_page('test_part1', 0); -- get farther and error about empty table
drop table test_partitioned;
+
+-- check null bitmap alignment for table whose the number of attributes is multiple of 8
+create table test8 (f1 int, f2 int, f3 int, f4 int, f5 int, f6 int, f7 int, f8 int);
+insert into test8(f1, f8) values (x'f1'::int, 0);
+select t_bits, t_data from heap_page_items(get_raw_page('test8', 0));
+select tuple_data_split('test8'::regclass, t_data, t_infomask, t_infomask2, t_bits)
+ from heap_page_items(get_raw_page('test8', 0));
+drop table test8;
* Re: [HACKERS] wrong t_bits alignment in pageinspect
@ 2018-01-04 19:03 Andrey Borodin <x4mmm@yandex-team.ru>
parent: Maksim Milyutin <milyutinma@gmail.com>
1 sibling, 0 replies; 5+ messages in thread
From: Andrey Borodin @ 2018-01-04 19:03 UTC (permalink / raw)
To: pgsql-hackers; +Cc: Maksim Milyutin <milyutinma@gmail.com>
The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: not tested
Spec compliant: tested, passed
Documentation: tested, passed
Everything is fixed, works properly and I have no new notices.
I think that patch is ready for committer.
Best regards, Andrey Borodin.
The new status of this patch is: Ready for Committer
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: [HACKERS] wrong t_bits alignment in pageinspect
@ 2018-01-04 20:03 Tom Lane <tgl@sss.pgh.pa.us>
parent: Maksim Milyutin <milyutinma@gmail.com>
1 sibling, 0 replies; 5+ messages in thread
From: Tom Lane @ 2018-01-04 20:03 UTC (permalink / raw)
To: Maksim Milyutin <milyutinma@gmail.com>; +Cc: Andrey Borodin <x4mmm@yandex-team.ru>; pgsql-hackers
Maksim Milyutin <milyutinma@gmail.com> writes:
> Attached a new version of patch with regression test.
Looks good, pushed.
I also removed one error check from tuple_data_split --- there doesn't
seem to be a lot of point in checking that bits_str_len is some multiple
of 8 when we're about to check that it's equal to a particular multiple
of 8.
regards, tom lane
^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2018-01-04 20:03 UTC | newest]
Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2017-12-15 13:53 [HACKERS] wrong t_bits alignment in pageinspect Maksim Milyutin <milyutinma@gmail.com>
2018-01-02 09:33 ` Andrey Borodin <x4mmm@yandex-team.ru>
2018-01-04 18:06 ` Maksim Milyutin <milyutinma@gmail.com>
2018-01-04 19:03 ` Andrey Borodin <x4mmm@yandex-team.ru>
2018-01-04 20:03 ` Tom Lane <tgl@sss.pgh.pa.us>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox