pageinspect: add tuple_data_record()

Hi,

On 2018-10-16 21:39:02 -0400, James Coleman wrote:

Background:
In my usage of pageinspect for debugging or other purposes I often find it
frustrating that I don't have a way to easily view a heap page's tuple data
as actual records rather than binary data. After encountering this again
last week while doing some data recovery after an accidental delete (and
manually parsing a few int and timestamp fields with get_byte magic) I
decided to write up a patch to add a function to handle this case.

Summary:
The new function tuple_data_record() parallels the existing
tuple_data_split() function, but instead of returning a bytea array of raw
attribute heap values, it returns a row type of the relation being examined.

I don't think it's entirely safe to do so for invisible rows. The toast
references could be reused, constraints be violated, etc. So while I
could have used this several times before, it's also very likely a good
way to cause trouble. It'd probably be ok to just fetch the binary
value of the columns, but detoasting sure ain't ok.

This is my first patch submission, so I encountered a few questions that
the coding style docs didn't seem to address, like whether it's preferable
to line up variable declarations by name.

There's ./src/tools/pgindent/pgindent (which needs an external dep), to
do that kind of thing...

Greetings,

Andres Freund

I don't think it's entirely safe to do so for invisible rows. The toast
references could be reused, constraints be violated, etc. So while I
could have used this several times before, it's also very likely a good
way to cause trouble. It'd probably be ok to just fetch the binary
value of the columns, but detoasting sure ain't ok.

Wouldn’t that be equally true for the already existing toast option to
tuple_data_split()?

Is “unsafe” here something that would lead to a crash? Or just returning
invalid data? Given that pageinspect is already clearly an internal viewing
of data, I think allowing invalid or limited data is a reasonable result.

Alternatively, would returning only inline values be safe (and say,
returning null for any toasted data)?

This is my first patch submission, so I encountered a few questions that
the coding style docs didn't seem to address, like whether it's

preferable

to line up variable declarations by name.

There's ./src/tools/pgindent/pgindent (which needs an external dep), to
do that kind of thing...

Yes, though seems like maybe the style guide could be a bit more
descriptive in some of these areas to be more friendly to newcomers. In
contrast the wiki page for submitting a patch is extremely detailed.

Thanks,
James Coleman

On Tue, Oct 16, 2018 at 6:39 PM James Coleman <jtc331@gmail.com> wrote:

Summary:
The new function tuple_data_record() parallels the existing tuple_data_split() function, but instead of returning a bytea array of raw attribute heap values, it returns a row type of the relation being examined.

I've been doing something similar with a modified
btreefuncs.c/bt_page_items() recently. The approach I've taken is
pretty grotty. It would be nice to have a reasonable way of doing
this, for both heap tuples and index tuples. (Relatedly,
bt_page_items() should probably have a bytea "data" field instead of a
text "data" field.)

--
Peter Geoghegan

Hi,

On 2018-10-16 22:05:28 -0400, James Coleman wrote:

I don't think it's entirely safe to do so for invisible rows. The toast
references could be reused, constraints be violated, etc. So while I
could have used this several times before, it's also very likely a good
way to cause trouble. It'd probably be ok to just fetch the binary
value of the columns, but detoasting sure ain't ok.

Wouldn’t that be equally true for the already existing toast option to
tuple_data_split()?

Yes. Whoever added that didn't think far enough. Teodor, Nikolay?

Is “unsafe” here something that would lead to a crash? Or just returning
invalid data? Given that pageinspect is already clearly an internal viewing
of data, I think allowing invalid or limited data is a reasonable result.

Both.

Alternatively, would returning only inline values be safe (and say,
returning null for any toasted data)?

I think that'd be somewhat confusing.

I don't think it's necessarily just toast that's a problem here, that
was just an obvious point.

Yes, though seems like maybe the style guide could be a bit more
descriptive in some of these areas to be more friendly to newcomers. In
contrast the wiki page for submitting a patch is extremely detailed.

I think there's a lot in that area we should improve.

Greetings,

Andres Freund

В письме от 16 октября 2018 19:17:20 пользователь Andres Freund написал:

Hi,

On 2018-10-16 22:05:28 -0400, James Coleman wrote:

I don't think it's entirely safe to do so for invisible rows. The toast
references could be reused, constraints be violated, etc. So while I
could have used this several times before, it's also very likely a good
way to cause trouble. It'd probably be ok to just fetch the binary
value of the columns, but detoasting sure ain't ok.

Wouldn’t that be equally true for the already existing toast option to
tuple_data_split()?

Yes. Whoever added that didn't think far enough. Teodor, Nikolay?

I did compleatly got the question... The question is it safe to split tuple
record into array of raw bytea? It is quite safe from my point of view. We
use only data that is inside the tuple, and info from pg_catalog that
describes the tuple structure. So we are not affected if for example toast
table were cleaned by vacuum. If you try to deTOAST data when TOAST table were
already overwritten by new data, you can get some trouble...

Is “unsafe” here something that would lead to a crash? Or just returning
invalid data?

I think that you should try and see. Find a way to make postgres vacuum toast
table, and overwrite it with some new data, but keep tuple in heap page
untouched. Then try to deTOAST old data and see what happened. (Hint: it is
good to turn off compression for your data to be TOASTed, so you can just look
into toast-file in binary mode and get an idea what in it now)

======

Summary:
The new function tuple_data_record() parallels the existing
tuple_data_split() function, but instead of returning a bytea array of raw
attribute heap values, it returns a row type of the relation being
examined.

I find this approach a bit wrong. pageinspect is about viewing data in raw
mode, not about looking at the tuples that were deleted. It can be used for
this task, but it has other main purpose.

I would suggest instead of automatically detoast and uncompress actual data, I
would add functions that:

1. Checks if TOAST id is valid, and data can be fetched
2. Fetches that data and print it in raw mode
3. Says if binary data is compressed varlen data
4. Uncompress binary data and return it as another binary data
5. Convert binary data into string.

or something like that. This would be more according to the pageinspect
spirit, as I see it.

Than using these functions you can write pure sql wrapper that do what you
really want.

--
Do code for fun.

I did compleatly got the question... The question is it safe to split
tuple
record into array of raw bytea? It is quite safe from my point of view.
We
use only data that is inside the tuple, and info from pg_catalog that
describes the tuple structure. So we are not affected if for example toast
table were cleaned by vacuum. If you try to deTOAST data when TOAST table
were
already overwritten by new data, you can get some trouble...

The existing tuple_data_split() method already explicitly allows deTOASTing
data,
so if this is a problem, the problem already exists in pageinspect.

I find this approach a bit wrong. pageinspect is about viewing data in raw
mode, not about looking at the tuples that were deleted. It can be used
for
this task, but it has other main purpose.

...

Than using these functions you can write pure sql wrapper that do what you
really want.

It's actually extremely difficult to extract real data types from the raw
disk data in pure
SQL, which was the reason for this patch.

When using pageinspect to investigate the raw data behind a potential bug
or other issue
(for example I've used it in the past pre-9.6 to check the xmin/xmax
information to help
track down replication bugs (determining if the tuple information was there
but just not
visible after a vacuum or if the tuple data itself was missing). Even in
this kind of "under
the hood" inspection it's quite useful to be able to see actual data types
rather than binary
data.

In these kinds of use cases I believe that just ignoring TOASTed data is
even acceptable
because once you're already down the rabbit hole of looking at non-visible
information
you've already accepted that the data may no longer be complete or valid.

В письме от 17 октября 2018 12:36:54 пользователь James Coleman написал:

I did compleatly got the question... The question is it safe to split
tuple
record into array of raw bytea? It is quite safe from my point of view.
We
use only data that is inside the tuple, and info from pg_catalog that
describes the tuple structure. So we are not affected if for example toast
table were cleaned by vacuum. If you try to deTOAST data when TOAST table
were
already overwritten by new data, you can get some trouble...

The existing tuple_data_split() method already explicitly allows deTOASTing
data,
so if this is a problem, the problem already exists in pageinspect.

Oh. that's true... I do not remember writing it, but it seems I did it. After
Andreas notion, I am sure this should be fixed by removing de_toast option for
tuple_data_split... O_o

--
Do code for fun.

On 2018-10-17 12:36:54 -0400, James Coleman wrote:

I did compleatly got the question... The question is it safe to split
tuple
record into array of raw bytea? It is quite safe from my point of view.
We
use only data that is inside the tuple, and info from pg_catalog that
describes the tuple structure. So we are not affected if for example toast
table were cleaned by vacuum. If you try to deTOAST data when TOAST table
were
already overwritten by new data, you can get some trouble...

The existing tuple_data_split() method already explicitly allows deTOASTing
data,
so if this is a problem, the problem already exists in pageinspect.

Indeed. But I do think your approach - which means that the binary data is
actually interpreded as a datum of a specific type, drastically
increases the risk.

Greetings,

Andres Freund

Indeed. But I do think your approach - which means that the binary data is

actually interpreded as a datum of a specific type, drastically
increases the risk.

Agreed.

As I noted earlier, I don't at all think deTOASTing is a must for this
function to be
valuable, just as tuple_data_split() is also valuable without deTOASTINGing.

I believe "best effort" is very reasonable in the case of a what is an
investigatory
method to begin with.

Hi,

On 2018-10-17 13:04:33 -0400, James Coleman wrote:

Indeed. But I do think your approach - which means that the binary data is

actually interpreded as a datum of a specific type, drastically
increases the risk.

Agreed.

As I noted earlier, I don't at all think deTOASTing is a must for this
function to be
valuable, just as tuple_data_split() is also valuable without deTOASTINGing.

I believe "best effort" is very reasonable in the case of a what is an
investigatory
method to begin with.

It's far from only toast that could be problematic here.

Greetings,

Andres Freund

It's far from only toast that could be problematic here.

Do you have an example in mind? Wouldn’t the tuples have to be corrupted in
some way to have problems with being interpreted as a datum? Or are you
thinking very old tuples with a radically different structure to be causing
the problem?

Hi,

On 2018-10-17 13:14:17 -0400, James Coleman wrote:

It's far from only toast that could be problematic here.

Do you have an example in mind? Wouldn’t the tuples have to be corrupted in
some way to have problems with being interpreted as a datum? Or are you
thinking very old tuples with a radically different structure to be causing
the problem?

There's plenty ways it can go horribly wrong. Let's start with something
simple:

BEGIN;
ALTER TABLE ... ADD COLUMN blarg INT;
INSERT ... (blag) VALUES (132467890);
ROLLBACK;

ALTER TABLE ... ADD COLUMN blarg TEXT;

If you now read the table with your function you'll see a dead row that
will re-interpret a int datum as a text datum. Which in all likelyhood
will crash the server.

And yes, it's more likely to break with your patch, but I think the
current *split* function is dangerous too, and shouldn't have been added
without *A LOT* more attention to issues like this.

Greetings,

Andres Freund

There's plenty ways it can go horribly wrong. Let's start with something
simple:

BEGIN;
ALTER TABLE ... ADD COLUMN blarg INT;
INSERT ... (blag) VALUES (132467890);
ROLLBACK;

ALTER TABLE ... ADD COLUMN blarg TEXT;

If you now read the table with your function you'll see a dead row that
will re-interpret a int datum as a text datum. Which in all likelyhood
will crash the server.

That particular case gives this result:
ERROR: number of attributes in tuple header is greater than number of
attributes in tuple descriptor

Some more extended monkeying with adding/dropping columns repeatedly
gave this result:
ERROR: unexpected end of tuple data

That error (unexpected end of tuple data) should (at least in the non-TOAST
case)
prevent the bug of reading beyond the raw tuple data in memory, which would
be
the easiest way I could imagine to cause a serious problem.

Is there a case that could crash outside of a non-primitive type that has
unsafe
data reading code?

On 2018-10-17 17:02:20 -0400, James Coleman wrote:

There's plenty ways it can go horribly wrong. Let's start with something
simple:

BEGIN;
ALTER TABLE ... ADD COLUMN blarg INT;
INSERT ... (blag) VALUES (132467890);
ROLLBACK;

ALTER TABLE ... ADD COLUMN blarg TEXT;

If you now read the table with your function you'll see a dead row that
will re-interpret a int datum as a text datum. Which in all likelyhood
will crash the server.

That particular case gives this result:
ERROR: number of attributes in tuple header is greater than number of
attributes in tuple descriptor

I don't see why you'd get that error, if you re-add a column (with a
different type), as I did above? Obviously the "replacement" column
addition would need to be committed.

Some more extended monkeying with adding/dropping columns repeatedly
gave this result:
ERROR: unexpected end of tuple data

That error (unexpected end of tuple data) should (at least in the non-TOAST
case)
prevent the bug of reading beyond the raw tuple data in memory, which would
be
the easiest way I could imagine to cause a serious problem.

You don't need to read beyond the end of the data. You just need to do
something like reinterpret types, where the original type looks enough
like a toast header (e.g.) to cause problems.

Is there a case that could crash outside of a non-primitive type that has
unsafe data reading code?

Just about anything that's not a fixed length type would be unsafe.

Greetings,

Andres Freund

I don't see why you'd get that error, if you re-add a column (with a
different type), as I did above? Obviously the "replacement" column
addition would need to be committed.

Here's my test case:
CREATE TABLE t3(i INTEGER);

BEGIN;
ALTER TABLE t3 ADD COLUMN blarg INT;
INSERT INTO t3(blarg) VALUES (132467890);
ROLLBACK;

ALTER TABLE t3 ADD COLUMN blarg TEXT;

SELECT *, tuple_data_split(
't3'::regclass, t_data, t_infomask, t_infomask2, t_bits
) FROM heap_page_items(get_raw_page('t3', 0));

SELECT tdr.*, blarg IS NULL
FROM heap_page_items(get_raw_page('t3', 0))
LEFT JOIN LATERAL (
SELECT *
FROM tuple_data_record(
't3'::regclass, t_data, t_infomask, t_infomask2, t_bits
) AS n(i INTEGER, blarg TEXT)
) tdr ON true;

DROP TABLE t3;

and here's the output:

CREATE TABLE
BEGIN
ALTER TABLE
INSERT 0 1
ROLLBACK
ALTER TABLE
psql:test.sql:12: ERROR: unexpected end of tuple data
psql:test.sql:21: ERROR: unexpected end of tuple data
DROP TABLE

That error (unexpected end of tuple data) should (at least in the

non-TOAST

case)
prevent the bug of reading beyond the raw tuple data in memory, which

would

be
the easiest way I could imagine to cause a serious problem.

You don't need to read beyond the end of the data. You just need to do
something like reinterpret types, where the original type looks enough
like a toast header (e.g.) to cause problems.

Is there a case that could crash outside of a non-primitive type that has
unsafe data reading code?

Just about anything that's not a fixed length type would be unsafe.

Let's use the example of TEXT (that isn't TOASTed). If some bytes
are incorrectly interpreted as variable length text, then you have two
possibilities that I can see:

1. The determined length is less than the fixed type's space, in which
case we get bogus interpretation but it is not unsafe.
2. The determined length is greater than the fixed type's space. In this
case we get either bogus interpretation (of data beyond the fixed type's
space) or we get an error (like above).

Assuming bounds checking disallows reading beyond the raw tuple
data, then I still do not understand how a variable length field like
text (outside of TOAST at least) could be actually unsafe. It obviously
could give bizarre data.