Re: [HACKERS] CREATE RULE _RETURN and toast tables
Andres Freund and...@2ndquadrant.com writes: On 2013-02-14 20:47:11 -0500, Tom Lane wrote: Andres Freund and...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] CREATE RULE _RETURN and toast tables
On 2013-02-14 20:47:11 -0500, Tom Lane wrote:
Andres Freund and...@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
diff --git a/src/backend/rewrite/rewriteDefine.c b/src/backend/rewrite/rewriteDefine.c
index b37f36b..7e7b16a 100644
--- a/src/backend/rewrite/rewriteDefine.c
+++ b/src/backend/rewrite/rewriteDefine.c
@@ -16,6 +16,9 @@
#include access/heapam.h
#include access/htup_details.h
+#include access/transam.h
+#include access/multixact.h
+#include access/xact.h
#include catalog/catalog.h
#include catalog/dependency.h
#include catalog/heap.h
@@ -500,30 +503,101 @@ DefineQueryRewrite(char *rulename,
replace);
/*
- * Set pg_class 'relhasrules' field TRUE for event relation. If
- * appropriate, also modify the 'relkind' field to show that the
- * relation is now a view.
+ * Set pg_class 'relhasrules' field TRUE for event relation.
*
* Important side effect: an SI notice is broadcast to force all
* backends (including me!) to update relcache entries with the new
* rule.
*/
- SetRelationRuleStatus(event_relid, true, RelisBecomingView);
+ SetRelationRuleStatus(event_relid, true);
}
- /*
- * If the relation is becoming a view, delete the storage files associated
- * with it. Also, get rid of any system attribute entries in pg_attribute,
- * because a view shouldn't have any of those.
+ /* -
+ * If the relation is becoming a view
+ * - delete the associated storage files
+ * - get rid of any system attributes in pg_attribute, a view shouldn't
+ have any of those
+ * - remove the toast table, there is no need for it anymore, and its
+ presence would make vacuum slightly more complicated
+ * - set relkind to RELKIND_VIEW
+ * - adjust other pg_class attributes to be appropriate for a view
*
* NB: we had better have AccessExclusiveLock to do this ...
- *
- * XXX what about getting rid of its TOAST table? For now, we don't.
+ * -
*/
if (RelisBecomingView)
{
+ Relation relationRelation;
+ Oid toastrelid;
+ HeapTuple classTup;
+ Form_pg_class classForm;
+
+ relationRelation = heap_open(RelationRelationId, RowExclusiveLock);
+ toastrelid = event_relation-rd_rel-reltoastrelid;
+
+ /* drop storage while table still looks like a table */
RelationDropStorage(event_relation);
DeleteSystemAttributeTuples(event_relid);
+
+ /*
+ * Now drop the toast table which is not needed anymore, the pg_class
+ * entry is adapted below.
+ */
+ if (toastrelid != InvalidOid)
+ {
+ ObjectAddress toastobject;
+
+ /*
+ * delete the dependency of the main relation to the toast relation
+ * so we can delete the toast relation without also deleting what
+ * is becoming the view.
+ */
+ deleteDependencyRecordsFor(RelationRelationId, toastrelid,
+ false);
+
+ /* make deletion of dependency record visible */
+ CommandCounterIncrement();
+
+ /* now drop toast table, including index */
+ toastobject.classId = RelationRelationId;
+ toastobject.objectId = toastrelid;
+ toastobject.objectSubId = 0;
+ performDeletion(toastobject, DROP_RESTRICT, PERFORM_DELETION_INTERNAL);
+ }
+
+ /*
+ * Fixup pg_class entry to look like a normal view's, including setting
+ * the correct relkind and removal of reltoastrelid, reltoastidxid of
+ * the toast table we potentially removed above.
+ */
+
+ /*
+ * SetRelationRuleStatus may have updated the pg_class row, so make
+ * current version visible before we fetch the current tuple.
+ */
+ CommandCounterIncrement();
+
+ classTup = SearchSysCacheCopy1(RELOID, ObjectIdGetDatum(event_relid));
+ if (!HeapTupleIsValid(classTup))
+ elog(ERROR, cache lookup failed for relation %u, event_relid);
+
+ classForm = (Form_pg_class) GETSTRUCT(classTup);
+ classForm-reltablespace = InvalidOid;
+ classForm-relpages = 0;
+ classForm-reltuples = 0;
+ classForm-relallvisible = 0;
+ classForm-reltoastrelid = InvalidOid;
+ classForm-reltoastidxid =
[HACKERS] 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 the view 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] CREATE RULE _RETURN and toast tables
Andres Freund and...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] CREATE RULE _RETURN and toast tables
On 2013-02-14 20:47:11 -0500, Tom Lane wrote: Andres Freund and...@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::bigint32))::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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] CREATE RULE _RETURN and toast tables
Andres Freund and...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] CREATE RULE _RETURN and toast tables
* Tom Lane (t...@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 signature.asc Description: Digital signature
Re: [HACKERS] CREATE RULE _RETURN and toast tables
Stephen Frost sfr...@snowman.net writes: * Tom Lane (t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] CREATE RULE _RETURN and toast tables
* Tom Lane (t...@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 signature.asc Description: Digital signature
