Thread: vac_update_datfrozenxid will raise "wrong tuple length" if pg_database tuple contains toast attribute.

CREATE DATABASE vacuum_freeze_test;

-- create helper function

create or replace function toast_pg_database_datacl() returns text as $body$

declare

        mycounter int;

begin

        for mycounter in select i from generate_series(1, 2800) i loop

                execute 'create role aaaabbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb' || mycounter;

                execute 'grant ALL on database vacuum_freeze_test to aaaabbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb' || mycounter;

        end loop;

        return 'ok';

end;

$body$ language plpgsql volatile strict;

-- create roles and grant on the database

select toast_pg_database_datacl();

-- connect to the database

\c vacuum_freeze_test

-- chech the size of column datacl

select datname, pg_column_size(datacl) as datacl_size, age(datfrozenxid) from pg_database where datname='vacuum_freeze_test';

-- execute vacuum freeze and it should raise "wrong tuple length"

vacuum freeze;

On Tue, Nov 24, 2020 at 2:07 PM Junfeng Yang <yjerome@vmware.com> wrote:
>
> Hi hackers,
>
> Can anyone help to verify this?
>

I think one way to get feedback is to register this patch for the next
commit fest (https://commitfest.postgresql.org/31/)

-- 
With Regards,
Amit Kapila.

On Wed, Nov 18, 2020 at 06:32:51AM +0000, Junfeng Yang wrote:
> A path is attached co auther by Ashwin Agrawal, the solution is to
> fetch the pg_database tuple from disk instead of system cache if
> needed.

Yeah, we had better fix and I guess backpatch something here.  That's
annoying.

+DROP DATABASE IF EXISTS vacuum_freeze_test;
+NOTICE:  database "vacuum_freeze_test" does not exist, skipping
+CREATE DATABASE vacuum_freeze_test;
This test is costly.  Extracting that into a TAP test would be more
adapted, still I am not really convinced that this is a case worth
spending extra cycles on.

+   tuple = systable_getnext(sscan);
+   if (HeapTupleIsValid(tuple))
+       tuple = heap_copytuple(tuple);
That's an unsafe pattern as the tuple scanned could finish by being
invalid.

One thing that strikes me as unwise is that we could run into similar
problems with vac_update_relstats() in the future, and there have been
recent talks about having more toast tables like for pg_class.  That
is not worth caring about on stable branches because it is not an
active problem there, but we could do something better on HEAD.

+   /* Get the pg_database tuple to scribble on */
+   cached_tuple = SearchSysCache1(DATABASEOID, ObjectIdGetDatum(MyDatabaseId));
+   cached_dbform = (Form_pg_database) GETSTRUCT(cached_tuple);
Er, shouldn't we *not* use the system cache at all here then?  Or am I
missing something obvious?  Please see the attached, that is more
simpleg.
--
Michael

On Wed, Nov 25, 2020 at 04:12:15PM +0900, Michael Paquier wrote:
> Yeah, we had better fix and I guess backpatch something here.  That's
> annoying.

I have considered that over the weekend, and let's be conservative.
pg_database has gained a toast table since 12, and before that we
would just have an error "row is too big", but nobody has really
complained about that either.

> One thing that strikes me as unwise is that we could run into similar
> problems with vac_update_relstats() in the future, and there have been
> recent talks about having more toast tables like for pg_class.  That
> is not worth caring about on stable branches because it is not an
> active problem there, but we could do something better on HEAD.

For now, I have added just a comment at the top of
heap_inplace_update() to warn callers.

Junfeng and Ashwin also mentioned to me off-list that their patch used
a second copy for performance reasons, but I don't see why this could
become an issue as we update each pg_database row only once a job is
done.  So I'd like to apply something like the attached on HEAD,
comments are welcome.
--
Michael

On Mon, Nov 30, 2020 at 04:37:14PM -0800, Ashwin Agrawal wrote:
> I am thinking if there is some way to assert this aspect, but seems no way.
> So, yes, having at least a comment is good for now.

Yeah, I have been thinking about this one without coming to a clear
idea, but that would be nice.

> Yes the attached patch looks good to me for PostgreSQL. Thanks Michael.

Okay, committed to HEAD then.
--
Michael

Hi Michael,

I got a customer case that matches this issue. The customer is on an
old version of 12, but I see the patch only went into 14:

On Tue, 8 Dec 2020 at 00:32, Michael Paquier <michael@paquier.xyz> wrote:
> > Yes the attached patch looks good to me for PostgreSQL. Thanks Michael.
> Okay, committed to HEAD then.

commit 947789f1f5fb61daf663f26325cbe7cad8197d58
Author: Michael Paquier <michael@paquier.xyz>
Date:   Tue Dec 8 12:13:19 2020 +0900

   Avoid using tuple from syscache for update of pg_database.datfrozenxid

Would it be possible to backport the patch to 12 and 13?

-- 
Arthur Nascimento
EDB

On Thu, Jan 19, 2023 at 10:42:47AM -0300, Arthur Nascimento wrote:
> Would it be possible to backport the patch to 12 and 13?

This was recently back-patched [0] [1] [2].

[0] https://postgr.es/m/Y70XNVbUWQsR2Car@paquier.xyz
[1] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=c0ee694
[2] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=72b6098

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Thu, 19 Jan 2023 at 14:10, Nathan Bossart <nathandbossart@gmail.com> wrote:
> This was recently back-patched [0] [1] [2].

Oh, I see that now. Thanks! Sorry for the noise.

--
Arthur Nascimento
EDB