Thread: CREATE RULE "_RETURN" and toast tables
Hi,
While investigating an anti-wraparound shutdown issue of Peter
H. Ezetta (cced) on IRC the issue came up that when you:
CREATE TABLE foo(id int, text text);
CREATE RULE "_RETURN" AS ON SELECT TO foo DO INSTEAD SELECT 1::int AS id, ''::text AS text;
a) the view keeps its relfrozenxid value
b) the toast table remains
Peter is running (or rather migrating away from) 8.3 and in 8.3 toast
tables cannot be vacuumed by
1) manual VACUUMS, since vacuum() passes RELKIND_RELATION to vacuum_rel() which thus errors out when vacuuming either
theview or the toast table directly:if (onerel->rd_rel->relkind != expected_relkind){ ereport(WARNING,
(errmsg("skipping\"%s\" --- cannot vacuum indexes, views, or special system tables",
RelationGetRelationName(onerel))));
2) autovacuum recognizes that the toast table needs vacuuming but uses
the following brute force trick to search for the table to find the
relation to vacuum:foreach(cell, toast_oids){ Oid toastoid = lfirst_oid(cell); ListCell *cell2;
foreach(cell2, table_toast_list) { av_relation *ar = lfirst(cell2);
if (ar->ar_toastrelid == toastoid) { table_oids = lappend_oid(table_oids, ar->ar_relid);
break; } }}
due to no respective element being in in table_toast_list nothing is
vacuumed and you cannot escape the situation. Not very nice. I wonder if
we should do something about it even due 8.3 is formally out of support,
not being able to migrate away from 8.3 because it shutdown is kinda
bad.
Due to some lucky coding 8.4+'s autovacuum (I tested only HEAD, but the
code in 8.4 looks fine) manages to vacuum the toast relation even though
no main table exists for it as it only consults the mapping for autovac
options. Its now also allowed to directly vacuum toast tables.
The current behaviour doesn't seem to be a terribly good idea. I propose
to drop the toast table and reset the relfrozenxid in DefineQueryRewrite
in the RelisBecomingView case. Currently the code only does: * * XXX what about getting rid of its TOAST table? For
now,we don't. */if (RelisBecomingView){ RelationDropStorage(event_relation);
DeleteSystemAttributeTuples(event_relid);}
Dropping the toast table seems like its important, it currently only
works by accident, I really doubt everbody working on (auto-)vacuum is
aware of that case.
I would also vote for resetting relfrozenxid of the main relation, but
thats more of a cosmetical issue.
Opinions?
Greetings,
Andres Freund
-- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services
Andres Freund <andres@2ndquadrant.com> writes:
> due to no respective element being in in table_toast_list nothing is
> vacuumed and you cannot escape the situation. Not very nice. I wonder if
> we should do something about it even due 8.3 is formally out of support,
Out of support is out of support. We're certainly not going to update
8.3 to fix corner cases that escaped notice for the five years it was in
support. (And it's not true that you can't get out of it --- if nothing
else, you could manually update the toast table's relfrozenxid value.)
> The current behaviour doesn't seem to be a terribly good idea. I propose
> to drop the toast table and reset the relfrozenxid in DefineQueryRewrite
> in the RelisBecomingView case.
Yeah, probably worth doing. At the time we thought that that code path
was just a short-term legacy thing for loading ancient pg_dump files.
However, given that even modern pg_dumps will use this syntax if
necessary to break circular dependencies for views, we're probably never
going to be rid of it completely.
regards, tom lane
On 2013-02-14 20:47:11 -0500, Tom Lane wrote: > Andres Freund <andres@2ndquadrant.com> writes: > > due to no respective element being in in table_toast_list nothing is > > vacuumed and you cannot escape the situation. Not very nice. I wonder if > > we should do something about it even due 8.3 is formally out of support, > > Out of support is out of support. We're certainly not going to update > 8.3 to fix corner cases that escaped notice for the five years it was in > support. Well, its going to get more likely with age... But sure, I have no probelm > (And it's not true that you can't get out of it --- if nothing > else, you could manually update the toast table's relfrozenxid value.) Yea, thats what we ended up with. For the benefit of people searching for the problem, if you hit strange wraparound issues that cannot be fixed in 8.3 you can escape the issue with: UPDATE pg_class SET relfrozenxid = (txid_current() % (1::bigint<<32))::text::xid WHERE NOT relfrozenxid = '0' AND relkind = 't' AND pg_class.oid IN ( SELECT reltoastrelid FROM pg_class WHERE relkind= 'v') RETURNING *; > > The current behaviour doesn't seem to be a terribly good idea. I propose > > to drop the toast table and reset the relfrozenxid in DefineQueryRewrite > > in the RelisBecomingView case. > > Yeah, probably worth doing. At the time we thought that that code path > was just a short-term legacy thing for loading ancient pg_dump files. > However, given that even modern pg_dumps will use this syntax if > necessary to break circular dependencies for views, we're probably never > going to be rid of it completely. Yep, thats what I thought. Will write something up. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
Andres Freund <andres@2ndquadrant.com> writes:
> On 2013-02-14 20:47:11 -0500, Tom Lane wrote:
>> Yeah, probably worth doing. At the time we thought that that code path
>> was just a short-term legacy thing for loading ancient pg_dump files.
>> However, given that even modern pg_dumps will use this syntax if
>> necessary to break circular dependencies for views, we're probably never
>> going to be rid of it completely.
> Yep, thats what I thought. Will write something up.
BTW, it strikes me that we *could* get pg_dump to stop doing this if we
wanted. Instead of the CREATE TABLE/CREATE RULE hack, we could have it
create a dummy view with the right rowtype like so:
CREATE VIEW v AS SELECT null::typename1 AS colname1, null::typename2 AS colname2, ... ;
then dump whatever had the circular-dependency issue with the view's
rowtype, and finally use CREATE OR REPLACE VIEW to replace the dummy
definition with the proper one.
This wouldn't really have any short-term benefit --- in particular, it
doesn't relieve the pressure to fix DefineQueryRewrite as you propose.
The advantage is that in ten years or so there would be no pg_dump files
anywhere using CREATE RULE "_RETURN", and so we could hope to eventually
deprecate that syntax. Which would let us get rid of the
RelIsBecomingView code path, and maybe have a bit more wiggle room to
remove or redesign the rule system.
That payoff is a little bit too far off to motivate me to do anything in
this line personally, but in case anybody else is more excited about it,
I thought I'd get the idea into the archives.
regards, tom lane
* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> That payoff is a little bit too far off to motivate me to do anything in
> this line personally, but in case anybody else is more excited about it,
> I thought I'd get the idea into the archives.
Any objection to making it a TODO? Might be a bit light for a GSOC
project, but perhaps a beginner (or really modivated student who wanted
to show that they are willing, able, and excited to contribute..) will
pick it up.
Thanks,
Stephen
Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> That payoff is a little bit too far off to motivate me to do anything in
>> this line personally, but in case anybody else is more excited about it,
>> I thought I'd get the idea into the archives.
> Any objection to making it a TODO?
None here. I was thinking it might be a useful finger exercise for
someone who wanted to learn about pg_dump.
regards, tom lane
* Tom Lane (tgl@sss.pgh.pa.us) wrote: > > Any objection to making it a TODO? > > None here. I was thinking it might be a useful finger exercise for > someone who wanted to learn about pg_dump. Done. Thanks. Stephen
On 2013-02-14 20:47:11 -0500, Tom Lane wrote: > Andres Freund <andres@2ndquadrant.com> writes: > > The current behaviour doesn't seem to be a terribly good idea. I propose > > to drop the toast table and reset the relfrozenxid in DefineQueryRewrite > > in the RelisBecomingView case. > > Yeah, probably worth doing. At the time we thought that that code path > was just a short-term legacy thing for loading ancient pg_dump files. > However, given that even modern pg_dumps will use this syntax if > necessary to break circular dependencies for views, we're probably never > going to be rid of it completely. What about the attached patch? I chose to move the update of relkind from SetRelationRuleStatus to the RelisBecomingView part of DefineQueryRewrite. As we're updating pg_class in there anyway there doesn't seem to be any reason to spread knowledge of that any further. Andres -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Attachment
Andres Freund <andres@2ndquadrant.com> writes:
> On 2013-02-14 20:47:11 -0500, Tom Lane wrote:
>> Andres Freund <andres@2ndquadrant.com> writes:
>>> The current behaviour doesn't seem to be a terribly good idea. I propose
>>> to drop the toast table and reset the relfrozenxid in DefineQueryRewrite
>>> in the RelisBecomingView case.
>> Yeah, probably worth doing.
> What about the attached patch?
Applied with some cosmetic changes.
regards, tom lane