Drop database command will raise "wrong tuple length" if pg_database tuple contains toast attribute.

Hi Ayush,

On 8/13/24 07:37, Ayush Tiwari wrote:

Hi hackers,

We encountered an issue lately, that if the database grants too many
roles `datacl` is toasted, following which, the drop database command
will fail with error "wrong tuple length".

To reproduce the issue, please follow below steps:

CREATE DATABASE test;

-- create helper function
CREATE OR REPLACE FUNCTION data_tuple() returns text as $body$
declare
          mycounter int;
begin
          for mycounter in select i from generate_series(1,2000) i loop
                    execute 'CREATE
ROLE aaaaaaaaaaaaaaaaaaaaaaaaaaabbbbbbbbbbbbbbbbbbbbbb ' || mycounter;
                    execute 'GRANT ALL ON DATABASE test to
aaaaaaaaaaaaaaaaaaaaaaaaaaabbbbbbbbbbbbbbbbbbbbbb ' || mycounter;
          end loop;
          return 'ok';
end;
$body$ language plpgsql volatile strict; 

-- create roles and grant on the database.
SELECT data_tuple(); 

-- drop database command, this will result in "wrong tuple length" error.
DROP DATABASE test;

The root cause of this behaviour is that the HeapTuple in dropdb
function fetches a copy of pg_database tuple from system cache.
But the system cache flattens any toast attributes, which cause the
length check to fail in heap_inplace_update.

A patch for this issue is attached to the mail, the solution is to
change the logic to fetch the tuple by directly scanning pg_database
rather than using the catcache.

Thanks for the report. I can reproduce the issue following your
instructions, and the fix seems reasonable ...

But there's also one thing I don't quite understand. I did look for
other places that might have a similar issue, that is places that

1) lookup tuple using SearchSysCacheCopy1

2) call on the tuple heap_inplace_update

And I found about four places doing that:

- index_update_stats (src/backend/catalog/index.c)

- create_toast_table (src/backend/catalog/toasting.c)

- vac_update_relstats / vac_update_datfrozenxid (commands/vacuum.c)

But I haven't managed to trigger the same kind of failure for any of
those places, despite trying. AFAIK that's because those places update
pg_class, and that doesn't have TOAST, so the tuple length can't change.

So this fix seems reasonable.

--
Tomas Vondra

On 8/16/24 13:26, Tomas Vondra wrote:

Hi Ayush,

...

So this fix seems reasonable.

I've pushed this to all affected branches, except for 11 which is EOL.

I thought about adding a test, but I couldn't think of a TAP test where
this would really fit, and it didn't seem very practical to have a test
creating hundreds of roles. So I abandoned the idea.

Thanks for the report and the fix!

--
Tomas Vondra

On Mon, 19 Aug 2024 00:35:39 +0200
Tomas Vondra <tomas@vondra.me> wrote:

On 8/16/24 13:26, Tomas Vondra wrote:

Hi Ayush,

...

So this fix seems reasonable.

I've pushed this to all affected branches, except for 11 which is EOL.

I thought about adding a test, but I couldn't think of a TAP test where
this would really fit, and it didn't seem very practical to have a test
creating hundreds of roles. So I abandoned the idea.

I tried to add Assert in heap_inplace_update to prevent possible similar
failures, but I gave up because I could not find a good way to determine if
a tuple is detoasted of not.

By the way, I found a comment in vac_update_datfrozenxid() and EventTriggerOnLogin()
that explains why we could not use tuples from the syscache for heap_inplace_update.
I think it is better ad d the same comment in dropdb(). I attached a trivial patch for it.

Regards,
Yugo Nagata

--
Yugo Nagata <nagata@sraoss.co.jp>

On 8/19/24 11:01, Yugo Nagata wrote:

On Mon, 19 Aug 2024 00:35:39 +0200
Tomas Vondra <tomas@vondra.me> wrote:

On 8/16/24 13:26, Tomas Vondra wrote:

Hi Ayush,

...

So this fix seems reasonable.

I've pushed this to all affected branches, except for 11 which is EOL.

I thought about adding a test, but I couldn't think of a TAP test where
this would really fit, and it didn't seem very practical to have a test
creating hundreds of roles. So I abandoned the idea.

I tried to add Assert in heap_inplace_update to prevent possible similar
failures, but I gave up because I could not find a good way to determine if
a tuple is detoasted of not.

Right, not sure there's a good way to check for that.

By the way, I found a comment in vac_update_datfrozenxid() and EventTriggerOnLogin()
that explains why we could not use tuples from the syscache for heap_inplace_update.
I think it is better ad d the same comment in dropdb(). I attached a trivial patch for it.

Agreed. That seems like a nice improvement to the comment.

regards

--
Tomas Vondra

On 8/19/24 12:16, Tomas Vondra wrote:

On 8/19/24 11:01, Yugo Nagata wrote:

...

By the way, I found a comment in vac_update_datfrozenxid() and EventTriggerOnLogin()
that explains why we could not use tuples from the syscache for heap_inplace_update.
I think it is better ad d the same comment in dropdb(). I attached a trivial patch for it.

Agreed. That seems like a nice improvement to the comment.

Done, thanks for the suggestion / patch.

regards

--
Tomas Vondra